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

Fixes #20626 - Periodic execution plan cleaner #37

Merged
merged 4 commits into from Aug 24, 2017

Conversation

adamruzicka
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

Copy link
Member

@iNecas iNecas left a comment

Choose a reason for hiding this comment

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

I started testing this change, but then I've noticed that jobs that end up in failure (let's say the host is unreachable or the script's non-zere exit code), the execution plans end up in paused state, and would not get cleaned http://projects.theforeman.org/issues/20647.

I will proceed with testing together with the fixes in the issue above.

@@ -39,3 +39,5 @@

# Log level, one of UNKNOWN, FATAL, ERROR, WARN, INFO, DEBUG
# :log_level: ERROR

# :execution_plan_cleaner_age: 86400
Copy link
Member

Choose a reason for hiding this comment

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

New line missing

Copy link
Member

Choose a reason for hiding this comment

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

Also, please comment here the value is in seconds and the default is 24 hours :)

@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@iNecas
Copy link
Member

iNecas commented Aug 18, 2017

One thing that came out of the testing: the cleaner should emit some info log messages about it's activity: it can be very useful

@iNecas
Copy link
Member

iNecas commented Aug 18, 2017

The log needs to probably go into Dynflow

@@ -40,6 +40,7 @@ class Settings < OpenStruct
:plugins => {},
:pid_file => '/var/run/foreman-proxy/smart_proxy_dynflow_core.pid',
:daemonize => false,
:execution_plan_cleaner_age => 60 * 60 * 24,
Copy link
Member

Choose a reason for hiding this comment

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

You need to add this also to PLUGIN_SETTINGS for the code to properly load the data from settings

@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

1 similar comment
@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@iNecas
Copy link
Member

iNecas commented Aug 18, 2017

Tested and works well, after doing some changes mentioned in the review

@adamruzicka
Copy link
Contributor Author

Addressed comments, logging is handled in Dynflow/dynflow#248

@iNecas
Copy link
Member

iNecas commented Aug 21, 2017

I don't see the code that would address the comments: forgot to push?

@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@adamruzicka
Copy link
Contributor Author

Pushed now...

@@ -2,3 +2,4 @@
:enabled: true
:database: /var/lib/foreman-proxy/dynflow/dynflow.sqlite
:core_url: 'http://127.0.0.1:8008'
:cleaner_max_age: 86400 # 1 day
Copy link
Member

Choose a reason for hiding this comment

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

Why this one?

@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@iNecas
Copy link
Member

iNecas commented Aug 22, 2017

I've released dynflow 0.8.27, please bump the dependency here.

@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@theforeman-bot
Copy link
Member

@adamruzicka, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@iNecas
Copy link
Member

iNecas commented Aug 24, 2017

Test failures unrelated. Merging

@iNecas iNecas merged commit 9155de3 into theforeman:master Aug 24, 2017
@iNecas
Copy link
Member

iNecas commented Aug 24, 2017

Tested and works great. Thanks @adamruzicka

@adamruzicka adamruzicka deleted the periodic-cleaner branch August 24, 2017 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants