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

Refactor Cron and merge dashboard tasks #10745

Merged
merged 20 commits into from
May 16, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 16, 2020

  • Merge Cron and Dashboard tasks and always run in the background
  • Make every cron task report a system notice on completion
  • Refactor the creation of these tasks
  • Ensure that execution counts of tasks is correct
  • Allow cron tasks to be started from the cron page
  • Make cron tasks appear as processes in the process manager

Fix #10299

* Merge Cron and Dashboard tasks
* Make every cron task report a system notice on completion
* Refactor the creation of these tasks
* Ensure that execution counts of tasks is correct
* Allow cron tasks to be started from the cron page
@zeripath zeripath added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 16, 2020
@zeripath zeripath added this to the 1.12.0 milestone Mar 16, 2020
@lunny
Copy link
Member

lunny commented Mar 17, 2020

This PR still not resolve the probblem that multiple gitea instances will run tasks duplicated.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 17, 2020
@zeripath
Copy link
Contributor Author

@lunny I guess you mean if you have a cluster of giteas you only want one task running on one gitea at a time?

I think that would need a DB flag?

@guillep2k
Copy link
Member

Handling aborts properly is a problem in such a unique queue. Is Gitea currently prepared for multi-instance setups, or we're just trying to be future proof? Because if the later is true, we could just implement better queue implementations in future PRs.

@lunny
Copy link
Member

lunny commented Mar 19, 2020

@zeripath Out topic. My idea is we need a cluster mod, that there will be an etcd to store all the gitea servers information and one will be elected as master. Then only one instance(master) will create the cron task when the time is in. But the tasks will be queued, so that the tasks will be handled by all the gitea servers load balance.

For a standalone mode, only a master node, it will produce tasks when the time in and handle the tasks itself.

There is no DB flag, because all instances will have the shared DB data. But the tasks could be stored in database task table.

Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

Wow! It was intimidating, but not as hard to review as I thought. 😅

Just some items to clarify. It looks very good! 🎉

models/user.go Outdated Show resolved Hide resolved
modules/cron/cron.go Show resolved Hide resolved
modules/cron/tasks.go Outdated Show resolved Hide resolved
modules/cron/tasks.go Show resolved Hide resolved
modules/cron/tasks.go Show resolved Hide resolved

if config.IsEnabled() {
// We cannot use the entry return as there is no way to lock it
if _, err = c.AddJob(name, config.GetSchedule(), task); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I guess this will not duplicate or re-schedule tasks registered from previous instances (like those with @anually schedule). 😶

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH we need a database backend for these "@annually" scheduled tasks. That probably involves us either bringing cron in to Gitea or forking it.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

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

Just one nit, non blocking. LG-TM.

modules/cron/cron.go Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 8, 2020
modules/cron/cron.go Show resolved Hide resolved
modules/cron/cron.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 30, 2020

Codecov Report

Merging #10745 into master will increase coverage by 0.05%.
The diff coverage is 45.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10745      +/-   ##
==========================================
+ Coverage   43.84%   43.89%   +0.05%     
==========================================
  Files         607      611       +4     
  Lines       86893    87207     +314     
==========================================
+ Hits        38098    38283     +185     
- Misses      44093    44206     +113     
- Partials     4702     4718      +16     
Impacted Files Coverage Δ
models/error.go 31.94% <0.00%> (-0.51%) ⬇️
modules/auth/admin.go 0.00% <ø> (ø)
modules/git/git.go 35.95% <0.00%> (ø)
modules/repository/check.go 0.00% <0.00%> (ø)
modules/repository/hooks.go 20.30% <0.00%> (-0.16%) ⬇️
modules/setting/setting.go 45.19% <ø> (-0.09%) ⬇️
routers/admin/admin.go 11.30% <0.00%> (+1.01%) ⬆️
services/mirror/mirror.go 18.56% <0.00%> (-0.19%) ⬇️
models/user.go 49.18% <6.89%> (-0.49%) ⬇️
modules/cron/cron.go 25.00% <20.00%> (-22.20%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7c82cd...8f779b1. Read the comment docs.

@guillep2k
Copy link
Member

@lafriks can you review again?

@lunny
Copy link
Member

lunny commented May 2, 2020

I think we can create a timer events source to create tasks to queue and reuse our old queue style.

@zeripath
Copy link
Contributor Author

zeripath commented May 3, 2020

@lunny Mostly that's orthogonal to this work.

We'd need a different type of unique queue, one that leaves the item in the set until it's been processed. It's possible - we'd need to sort adjust the handler to remove the item as a defer after handling.

But as I say that's mostly orthogonal to these changes.

@zeripath
Copy link
Contributor Author

@lunny do you have any further comments?

@lafriks lafriks requested a review from lunny May 16, 2020 19:41
@lafriks
Copy link
Member

lafriks commented May 16, 2020

@zeripath should we move this to 1.13 also?

@zeripath
Copy link
Contributor Author

Unlike the other one I moved to 1.13 this is far less likely to cause problems.

I think it'd be a shame for this not to get in to 1.12.

@techknowlogick
Copy link
Member

I prefer this to get into 1.12 as well

@6543
Copy link
Member

6543 commented May 16, 2020

🚀

@techknowlogick techknowlogick merged commit 9a2e47b into go-gitea:master May 16, 2020
@zeripath zeripath deleted the refactor-cron branch May 17, 2020 00:42
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Jun 16, 2020
After go-gitea#10745 was merged, the number of System Notices increased obviously.
Add the type filter is useful to help users to quickly check out the useful
message they want.

Signed-off-by: a1012112796 <1012112796@qq.com>
@jolheiser jolheiser mentioned this pull request Jun 18, 2020
7 tasks
@somera
Copy link

somera commented Jun 18, 2020

  • Merge Cron and Dashboard tasks and always run in the background
  • Make every cron task report a system notice on completion
  • Refactor the creation of these tasks
  • Ensure that execution counts of tasks is correct
  • Allow cron tasks to be started from the cron page
  • Make cron tasks appear as processes in the process manager

Fix #10299

The GC isn't working in the 1.12.0 release.

@6543
Copy link
Member

6543 commented Jun 18, 2020

@somera can you create a new issue - referencing old pulls/issue is the way to go - but dont append new stuff to closed/merged one :)

@somera somera mentioned this pull request Jun 18, 2020
7 tasks
zeripath added a commit to zeripath/gitea that referenced this pull request Jul 4, 2020
Unfortunately go-gitea#10745 merged a spurious logging message. This PR removes this.

Signed-off-by: Andrew Thornton <art27@cantab.net>
lunny pushed a commit that referenced this pull request Jul 5, 2020
Unfortunately #10745 merged a spurious logging message. This PR removes this.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Lauris BH <lauris@nix.lv>
zeripath added a commit to zeripath/gitea that referenced this pull request Jul 5, 2020
Backport go-gitea#12139

Unfortunately go-gitea#10745 merged a spurious logging message. This PR removes this.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Lauris BH <lauris@nix.lv>
techknowlogick added a commit that referenced this pull request Jul 5, 2020
Backport #12139

Unfortunately #10745 merged a spurious logging message. This PR removes this.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Lauris BH <lauris@nix.lv>

Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
* Refactor Cron and merge dashboard tasks

* Merge Cron and Dashboard tasks
* Make every cron task report a system notice on completion
* Refactor the creation of these tasks
* Ensure that execution counts of tasks is correct
* Allow cron tasks to be started from the cron page

* golangci-lint fixes

* Enforce that only one task with the same name can be registered

Signed-off-by: Andrew Thornton <art27@cantab.net>

* fix name check

Signed-off-by: Andrew Thornton <art27@cantab.net>

* as per @guillep2k

* as per @lafriks

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Add git.CommandContext variants

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
Unfortunately go-gitea#10745 merged a spurious logging message. This PR removes this.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Lauris BH <lauris@nix.lv>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Garbage-Collection not working
9 participants