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 #36351 - Send out email notification when tasks linger in running or paused for longer than two days #716

Merged
merged 6 commits into from
Jun 20, 2023

Conversation

adamruzicka
Copy link
Contributor

@adamruzicka adamruzicka commented May 5, 2023

  • users should be able to opt-out/in
    • users are able to opt-in/opt-out in email preferences
    • toast notification is now tied to the email notification which isn't exactly great, but we don't really have any other means of configuring it
  • always enabled by default for admin
    • this is tricky. I thought I'd add a migration, but the migration needs to run after data (the template) is seeded. Currently the migration creates the template and then the seeds override it. I'm not particularly fond of this, but I don't really see another way
  • honor taxonomy boundaries
  • time should be configurable
    • done, sort of. We don't really have the means for doing this nicely as recurring logics cannot be edited
    • by default set to midnight
    • However, as a workaround the RL can be cancelled, env variable FOREMAN_TASKS_CHECK_LONG_RUNNING_TASKS_CRONLINE can be set to a new cronline and then on service restart the RL should get re-created with the new schedule
    • Alternatively rake foreman_tasks:reschedule_long_running_tasks_checker FOREMAN_TASKS_CHECK_LONG_RUNNING_TASKS_CRONLINE=$cron can be used
  • have a recurring task monitor the rest of tasks

@adamruzicka adamruzicka force-pushed the long-tasks branch 3 times, most recently from 2a9162d to 5c8f344 Compare June 9, 2023 12:24
adamruzicka and others added 3 commits June 11, 2023 11:59
Co-authored-by: Oleh Fedorenko <ofedoren@redhat.com>
A user is consider an admin if either:
- they have the admin flag set
- they have the Org admin role (or its clones) assigned
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @adamruzicka, code-wise LGTM, apart one small inline suggestion.

However, as a workaround the RL can be cancelled, env variable FOREMAN_TASKS_CHECK_LONG_RUNNING_TASKS_CRONLINE can be set to a new cronline and then on service restart the RL should get re-created with the new schedule

I wouldn't like server restart as the only option to change the task as a user :/ Could we maybe write this logic into a Rake task at least? I mean, we can? find the recurring task and stop? it and create a new one with the same ForemanTasks.register_scheduled_task(Actions::CheckLongRunningTasks, ENV['FOREMAN_TASKS_CHECK_LONG_RUNNING_TASKS_CRONLINE'] || '0 0 * * *')?

Also, a question for dummies: how can I test this out and not wait for 2 days?.. UPD: I guess I can simply change the INTERVAL to a different value 🤦. But since I've got that question: wouldn't a user want to change this interval to a e.g. 1 day? Where the 2 days value does come from?

db/seeds.d/95-mail_notifications.rb Outdated Show resolved Hide resolved
@adamruzicka
Copy link
Contributor Author

Could we maybe write this logic into a Rake task at least? I mean, we can? find the recurring task and stop?

Eh, yes, I'll get to it

how can I test this out and not wait for 2 days?

There are several ways. Changing the interval is one, manually updating a timestamp of a task in the db seems to work as well, but my all time favorite is using faketime

wouldn't a user want to change this interval to a e.g. 1 day?

Well, yes. Sadly there are quite a few things that would be nice if they were configurable per user (when it runs, the interval, disconnect toasts from emails), but we don't really have means to do so sensibly right now.

Where the 2 days value does come from?

The (now) linked BZ https://bugzilla.redhat.com/show_bug.cgi?id=1950836 mentions two days as the sweet spot for content things.

@adamruzicka
Copy link
Contributor Author

adamruzicka commented Jun 13, 2023

Ermm, what? Ah, it was never passing in the first place

@adamruzicka
Copy link
Contributor Author

I don't really see a sensible way how to have enable it by default for admin but make them able to opt-out. The migration idea falls flat because the user is added from seeds. If we put this into seeds then they won't really be able to opt-out.

@adamruzicka
Copy link
Contributor Author

Alright, after some internal discussions, we agreed to leave this always enabled for admins, where admins are either users explicitly marked as admins or users which have the org admin role assigned

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @adamruzicka, LGTM and thank you for waiting so long :/

@ofedoren ofedoren merged commit 03ce25b into theforeman:master Jun 20, 2023
3 of 4 checks passed
@adamruzicka adamruzicka deleted the long-tasks branch June 20, 2023 15:48
adamruzicka added a commit that referenced this pull request Jun 20, 2023
…ng or paused for longer than two days (#716)

* Fixes #36351 - Add email notifications

about long running tasks.

* Fixes #36351 - Create a recurring logic for task checking

* Fixes #36351 - Add notifications about long running tasks

* Refs #36351 - Add rake task for rescheduling

* Update db/seeds.d/95-mail_notifications.rb

Co-authored-by: Oleh Fedorenko <ofedoren@redhat.com>

* Refs #36351 - Always enable for admin

A user is consider an admin if either:
- they have the admin flag set
- they have the Org admin role (or its clones) assigned

---------

Co-authored-by: Oleh Fedorenko <ofedoren@redhat.com>
(cherry picked from commit 03ce25b)
ofedoren added a commit that referenced this pull request Jun 20, 2023
…in running or paused for longer than two days (#716)"

This reverts commit 03ce25b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants