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

Move restore repo to internal router and invoke from command to avoid open the same db file or queues files #15790

Merged
merged 4 commits into from
May 10, 2021

Conversation

lunny
Copy link
Member

@lunny lunny commented May 8, 2021

When execute the restore repo command and gitea is running, you may got errors to open queue files or sqlite database because the files have been locked.

This PR moves repo restore main logic to internal routers and send a http request to execute the command.

This also fixes the bug that units has been ignored from restore repository.

@lunny lunny added the type/bug label May 8, 2021
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 8, 2021
@codecov-commenter
Copy link

codecov-commenter commented May 8, 2021

Codecov Report

Merging #15790 (26078d0) into main (17a7797) will increase coverage by 0.03%.
The diff coverage is 24.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #15790      +/-   ##
==========================================
+ Coverage   43.97%   44.00%   +0.03%     
==========================================
  Files         678      681       +3     
  Lines       82047    82092      +45     
==========================================
+ Hits        36079    36126      +47     
+ Misses      40091    40087       -4     
- Partials     5877     5879       +2     
Impacted Files Coverage Δ
cmd/restore_repo.go 0.00% <0.00%> (ø)
modules/migrations/dump.go 0.00% <0.00%> (ø)
modules/private/restore_repo.go 0.00% <0.00%> (ø)
routers/private/restore_repo.go 0.00% <0.00%> (ø)
routers/routes/goget.go 70.21% <70.21%> (ø)
routers/private/internal.go 85.00% <100.00%> (+0.38%) ⬆️
routers/routes/web.go 90.84% <100.00%> (+4.35%) ⬆️
models/gpg_key.go 53.51% <0.00%> (-0.57%) ⬇️
... and 8 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 038e1db...26078d0. Read the comment docs.

modules/migrations/dump.go Show resolved Hide resolved
routers/private/restore_repo.go Outdated Show resolved Hide resolved
@lunny lunny force-pushed the lunny/move_restore_repo branch from c410b8d to 4bbf276 Compare May 9, 2021 07:00
@zeripath
Copy link
Contributor

zeripath commented May 9, 2021

OK one final context question.

Do we want these to be cancellable by cancelling the request or do we want them to just start the restoration in the background?

At the request looks to the user that the wait they're experiencing would imply that it is cancellable but it's not - and in fact the request will timeout after 60s because of the settings in modules/private - That will then look like it has failed but it will continue to do the migration in the background.

We could tie the migration to the myCtx.Req.GetContext() - and set no timeout - or we could stick it in the task queue or a separate go-routine.

@lunny
Copy link
Member Author

lunny commented May 9, 2021

OK. I think we can run it in another goroute and return immediately.

@lunny
Copy link
Member Author

lunny commented May 10, 2021

OK one final context question.

Do we want these to be cancellable by cancelling the request or do we want them to just start the restoration in the background?

At the request looks to the user that the wait they're experiencing would imply that it is cancellable but it's not - and in fact the request will timeout after 60s because of the settings in modules/private - That will then look like it has failed but it will continue to do the migration in the background.

We could tie the migration to the myCtx.Req.GetContext() - and set no timeout - or we could stick it in the task queue or a separate go-routine.

I have set the timeout as zero so that it will not timeout.

@zeripath
Copy link
Contributor

Ok so with the timeout at 0, we're now giving the impression that the request is cancellable and now we should use the myCtx.Req.Context() instead of the hammer context

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 10, 2021
@zeripath
Copy link
Contributor

We'll need to add some status updates in future but this is better.

@lunny lunny added this to the 1.15.0 milestone May 10, 2021
@lunny lunny merged commit e5723d6 into go-gitea:main May 10, 2021
@lunny lunny deleted the lunny/move_restore_repo branch May 10, 2021 07:57
lunny added a commit to lunny/gitea that referenced this pull request May 10, 2021
… open the same db file or queues files (go-gitea#15790)

* Move restore repo to internal router and invoke from command to avoid open the same db file or queues files

* Follow @zeripath's review

* set no timeout for resotre repo private request

* make restore repo cancelable
@lunny lunny added the backport/done All backports for this PR have been created label May 10, 2021
lunny added a commit that referenced this pull request May 10, 2021
… open the same db file or queues files (#15790) (#15816)

* Move restore repo to internal router and invoke from command to avoid open the same db file or queues files

* Follow @zeripath's review

* set no timeout for resotre repo private request

* make restore repo cancelable
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 10, 2021
… open the same db file or queues files (go-gitea#15790)

* Move restore repo to internal router and invoke from command to avoid open the same db file or queues files

* Follow @zeripath's review

* set no timeout for resotre repo private request

* make restore repo cancelable
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants