Fixes #30870 - Option to delete paused task. #407

patilsuraj767 · 2020-09-23T06:19:57Z

No description provided.

theforeman-bot · 2020-09-23T06:20:06Z

Issues: #30870

upadhyeammit

Functionality wise it works as expected. Code wise I am requesting Kavita for more views.

definitions/procedures/foreman_tasks/resume.rb

upadhyeammit · 2020-09-29T11:00:44Z

@kgaikwad request to review as changes are in core.

kgaikwad

Added few inline comments.

lib/foreman_maintain/runner.rb

kgaikwad · 2020-10-08T10:07:37Z

definitions/procedures/foreman_tasks/resume.rb

@@ -8,6 +8,8 @@ class Resume < ForemanMaintain::Procedure

    def run
      output << feature(:foreman_tasks).resume_task_using_hammer
+      puts '[ Waiting 30 seconds for resumed tasks to start. ]'
+      sleep 30


Will it be good to define constant at either procedure level or feature level?

Yes, we can do that.

kgaikwad · 2020-10-08T10:21:07Z

definitions/features/foreman_tasks.rb

@@ -176,6 +178,10 @@ def old_tasks_condition(state = "'stopped', 'paused'")
       "foreman_tasks_tasks.started_at < CURRENT_DATE - INTERVAL '#{MIN_AGE} days'"
  end

+  def paused_tasks_condition(state = "'paused'")
+    "foreman_tasks_tasks.state IN (#{state})"


Here, you have taken state of tasks into consideration.
not_paused check calls feature(:foreman_tasks).paused_tasks_count(ignored_tasks) to find specific paused tasks.
I think, deleting only such paused tasks where result=error and not having label from ignored_tasks is expected here. Could you confirm this behavior?

@kgaikwad Yes it is expected that we should not delete ignored_tasks
These tasks are meant to be running always.

@kgaikwad @patilsuraj767 Currently we delete tasks where result=error, what about the tasks where result is pending or delayed?

@jameerpathan111 There is another check in foreman-maintain. Check not_running task which is run after the check not_paused in upgrade workflow, it will take care of running/pending tasks.

kgaikwad · 2020-10-09T07:40:52Z

@patilsuraj767, @upadhyeammit
As I mentioned, changes were added to the core functionality which will change behaviour for all checks so it would be good to confirm this behaviour.

lib/foreman_maintain/reporter/cli_reporter.rb

jameerpathan111 · 2020-11-04T06:46:07Z

@patilsuraj767 I have tried this pr and it looks good to me:

Test steps:

have tasks with state=paused and result=error
foreman-maintain health check --label foreman-tasks-not-paused --assumeyes

Observation:

foreman-tasks-not-paused check offers new option i.e "Delete paused tasks"
When check is run with --assumeyes option it will first try to resume paused tasks(state=paused and result=error) and if it fails to do so it deletes them.

Workaround for creating paused tasks:

[root@dhcp-2-22 ~]# foreman-rake console
Loading production environment (Rails 5.2.1)          
irb(main):018:0> t = ForemanTasks::Task.where(id: ["1575bec3-c4be-44bb-8920-22fab7a917ba"]).first
=> #<ForemanTasks::Task::DynflowTask id: "12481c54-65f2-4fa5-9e98-38f0e4b158dd", type: "ForemanTasks::Task::DynflowTask", label: "Actions::Katello::ContentView::Publish", started_at: "2020-10-28 06:36:00", ended_at: "2020-10-28 06:36:05", state: "stopped", result: "success", external_id: "9ad9ff45-7558-49b9-8e84-d486c78abc2b", parent_task_id: nil, start_at: "2020-10-28 06:36:00", start_before: nil, action: "Publish content view 'dev'; organization 'Default ...", user_id: 4, state_updated_at: "2020-10-28 06:36:05">
irb(main):019:0> t.result = "error"
=> "error"
irb(main):020:0> t.state = "paused"
=> "paused"
irb(main):021:0> t.save(:validate => false)
=> true

FM command output:

--------------------------------------------------------------------------------
Check for paused tasks:                                               [FAIL]
There are currently 2 paused tasks in the system
--------------------------------------------------------------------------------
There are multiple steps to proceed:
1) Resume paused tasks
2) Delete paused tasks
3) Investigate the tasks via UI
(assuming option 1)
Resume paused tasks:                                                            
[ Waiting 30 seconds for resumed tasks to start. ]                    [OK]
Total tasks found paused in error state: 2
Total tasks resumed:                     0
Resumed tasks:                           

Total tasks failed to resume:            0
Failed tasks:                            

Total tasks skipped:                     2
Skipped tasks:                           
 1) Task identifier: 1575bec3-c4be-44bb-8920-22fab7a917ba
    Task action:     Publish
    Task errors:     Required lock is already taken by other running tasks.
    Please inspect their state, fix their errors and resume them.
    
    Required lock: read
    Conflicts with tasks:
    - https://satellite.example.com/foreman_tasks/tasks/89b39a65-ba0d-45aa-af4e-ad8201f2ad48
 2) Task identifier: 8cd065ba-9d99-4436-aa97-b788919dbe83
    Task action:     Publish
    Task errors:     Required lock is already taken by other running tasks.
    Please inspect their state, fix their errors and resume them.
    
    Required lock: read
    Conflicts with tasks:
    - https://satellite.example.com/foreman_tasks/tasks/89b39a65-ba0d-45aa-af4e-ad8201f2ad48
--------------------------------------------------------------------------------
Rerunning the check after fix procedure
Check for paused tasks:                                               [FAIL]
There are currently 2 paused tasks in the system
--------------------------------------------------------------------------------
There are multiple steps to proceed:
1) Resume paused tasks
2) Delete paused tasks
3) Investigate the tasks via UI
(assuming option 2)
Delete tasks:                                                                   
| Deleted paused stopped and paused tasks: 2                          [OK]      
--------------------------------------------------------------------------------
Rerunning the check after fix procedure
Check for paused tasks:                                               [OK]

jameerpathan111 · 2020-11-05T09:29:14Z

There was an issue I faced while trying this pr on 6.9. I am still looking into it, so for sometime would request to hold this pr from getting merged.

definitions/features/foreman_tasks.rb

definitions/procedures/foreman_tasks/resume.rb

upadhyeammit · 2020-12-21T06:29:32Z

@jameerpathan111 do you have any thoughts on latest changes ?

patilsuraj767 · 2020-12-22T13:04:52Z

definitions/checks/foreman_tasks/not_paused.rb

@@ -12,9 +12,17 @@ def run
      paused_tasks_count = feature(:foreman_tasks).paused_tasks_count(ignored_tasks)


@upadhyeammit According to me we should also remove ignored_tasks parameter from here. Because even though we can ignore these tasks they should not be in the Paused state ever, especially while satellite upgrade.
Let me know your thoughts on above.

I agree, however I have these thoughts,

This check is tagged as default which means it runs for health checks and at the time of upgrade checks and runs. So should we only skip ignored_task when we will actually do the upgrade check and upgrade run ?

We do this check after restarting the services and as per my limited knowledge if we restart all services these ignored tasks should not be in paused state ? So here if we delete those while doing the health check they wont be created again ? Because after deletion we are not restarting services ? However at the time of upgrade, at very end of scenario we start the services.

What you think @patilsuraj767 and @kgaikwad ?

Updates on above query:

Currently, this check is tagged as default so it is running as part of health as well as upgrade.
As per offline discussion with @upadhyeammit & @patilsuraj767, it is worth verifying few things before any solution:

What will be the impact when it deletes paused tasks including labels "Actions::Candlepin::ListenOnCandlepinEvents", "Actions::Katello::EventQueue::Monitor" because currently it is ignoring paused tasks with those labels?

Would it be safe to run this deletion procedure when it won't perform service restart?
Example - foreman-maintain health check command.

Do we really need this check as part of health command? Other tasks related checks are tagged as "pre-upgrade"

* What will be the impact when it deletes paused tasks including labels "Actions::Candlepin::ListenOnCandlepinEvents", "Actions::Katello::EventQueue::Monitor" because currently it is ignoring paused tasks with those labels?

delete all of these, these tasks were removed from Foreman Tasks in 6.7 and are no longer processed as a task. Definitely delete them..

* Would it be safe to run this deletion procedure when it won't perform service restart? Example - foreman-maintain health check command.

yes, any time you delete a paused task, you do not need to restart services.

* Do we really need this check as part of health command? Other tasks related checks are tagged as "pre-upgrade"

I'd keep this with the health check as it is now as well as during the upgrade.

upadhyeammit · 2021-01-05T06:57:33Z

@patilsuraj767 request to do necessary changes, thank you !

upadhyeammit · 2021-01-06T11:09:44Z

@kgaikwad any final thoughts before merging ?

upadhyeammit · 2021-01-06T13:59:47Z

I tested this and what I can see is we are printing sort of confusing message as below. I think we should only keep 'Deleted paused tasks' as message ? its coming from Procedures::ForemanTasks::Delete procedure.

There are multiple steps to proceed:
1) Resume paused tasks
2) Delete paused tasks
3) Investigate the tasks via UI
Select step to continue, [n(next)] 2
Delete tasks:                                                                   
\ Deleted paused stopped and paused tasks: 1                          [OK]      
--------------------------------------------------------------------------------
Rerunning the check after fix procedure
Check for paused tasks:                                               [OK]
--------------------------------------------------------------------------------
Check to verify no empty CA cert requests exist:                      [OK]
--------------------------------------------------------------------------------
Check whether system is self-registered or not:                       [OK]
--------------------------------------------------------------------------------

definitions/checks/foreman_tasks/not_paused.rb

upadhyeammit · 2021-01-07T11:58:50Z

definitions/checks/foreman_tasks/not_paused.rb

+
+    def next_procedures
+      if assumeyes?
+        [Procedures::ForemanTasks::Resume.new,


Can you explicitly return the array inside if block ? now even when we have assumeyes? true it returns three next steps.

theforeman-bot added Needs testing Not yet reviewed labels Sep 23, 2020

upadhyeammit suggested changes Sep 29, 2020

View reviewed changes

definitions/procedures/foreman_tasks/resume.rb Outdated Show resolved Hide resolved

definitions/procedures/foreman_tasks/resume.rb Outdated Show resolved Hide resolved

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Sep 29, 2020

kgaikwad suggested changes Oct 8, 2020

View reviewed changes

upadhyeammit reviewed Oct 12, 2020

View reviewed changes

lib/foreman_maintain/reporter/cli_reporter.rb Show resolved Hide resolved

upadhyeammit added the 6.9 label Oct 15, 2020

theforeman-bot added Needs re-review and removed Waiting on contributor labels Oct 21, 2020

upadhyeammit added the On Hold label Nov 19, 2020

theforeman-bot added the Waiting on contributor label Nov 24, 2020

patilsuraj767 force-pushed the delete-paused-task branch from 865555e to a913f06 Compare November 24, 2020 03:02

theforeman-bot removed the Waiting on contributor label Nov 24, 2020

upadhyeammit reviewed Nov 26, 2020

View reviewed changes

definitions/features/foreman_tasks.rb Outdated Show resolved Hide resolved

upadhyeammit reviewed Nov 26, 2020

View reviewed changes

definitions/procedures/foreman_tasks/resume.rb Outdated Show resolved Hide resolved

upadhyeammit added the Waiting on contributor label Nov 26, 2020

patilsuraj767 force-pushed the delete-paused-task branch from a15ae73 to ad9694b Compare December 7, 2020 07:30

theforeman-bot removed the Waiting on contributor label Dec 7, 2020

patilsuraj767 added 4 commits December 21, 2020 11:47

Fixes #30870 - Option to delete paused task.

68dfe7f

Refs #30870 - Added condition for UiInvestigate procedure

33a7efc

Refs #30870 - Using spinner to display sleep time.

d2c16fa

Refs #30870 - Added contant for wait time

d502f4e

patilsuraj767 force-pushed the delete-paused-task branch from ad9694b to d502f4e Compare December 21, 2020 06:22

patilsuraj767 commented Dec 22, 2020

View reviewed changes

Refs #30870 - Deleting all tasks

2f83320

kgaikwad reviewed Jan 6, 2021

View reviewed changes

definitions/checks/foreman_tasks/not_paused.rb Show resolved Hide resolved

patilsuraj767 added 2 commits January 7, 2021 12:31

Refs #30870 - Removed ignored_tasks method

a8a9dd8

Refs #30870 - Changed output message

7939e9d

upadhyeammit reviewed Jan 7, 2021

View reviewed changes

Refs #30870 - Fixed assumeyes return array

f3dccc7

upadhyeammit merged commit 616c23c into theforeman:master Jan 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #30870 - Option to delete paused task. #407

Fixes #30870 - Option to delete paused task. #407

patilsuraj767 commented Sep 23, 2020

theforeman-bot commented Sep 23, 2020

upadhyeammit left a comment

upadhyeammit commented Sep 29, 2020

kgaikwad left a comment

kgaikwad Oct 8, 2020

patilsuraj767 Oct 9, 2020

kgaikwad Oct 8, 2020

patilsuraj767 Oct 9, 2020

jameerpathan111 Nov 4, 2020

patilsuraj767 Nov 6, 2020

kgaikwad commented Oct 9, 2020

jameerpathan111 commented Nov 4, 2020 •

edited

jameerpathan111 commented Nov 5, 2020

upadhyeammit commented Dec 21, 2020

patilsuraj767 Dec 22, 2020

upadhyeammit Dec 23, 2020

kgaikwad Dec 30, 2020

mccun934 Jan 5, 2021

upadhyeammit commented Jan 5, 2021

upadhyeammit commented Jan 6, 2021

upadhyeammit commented Jan 6, 2021

upadhyeammit Jan 7, 2021

		@@ -12,9 +12,17 @@ def run
		paused_tasks_count = feature(:foreman_tasks).paused_tasks_count(ignored_tasks)

Fixes #30870 - Option to delete paused task. #407

Fixes #30870 - Option to delete paused task. #407

Conversation

patilsuraj767 commented Sep 23, 2020

theforeman-bot commented Sep 23, 2020

upadhyeammit left a comment

Choose a reason for hiding this comment

upadhyeammit commented Sep 29, 2020

kgaikwad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad commented Oct 9, 2020

jameerpathan111 commented Nov 4, 2020 • edited

jameerpathan111 commented Nov 5, 2020

upadhyeammit commented Dec 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upadhyeammit commented Jan 5, 2021

upadhyeammit commented Jan 6, 2021

upadhyeammit commented Jan 6, 2021

Choose a reason for hiding this comment

jameerpathan111 commented Nov 4, 2020 •

edited