Skip to content

Make Migrations Cancelable #12917

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

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,8 @@ var migrations = []Migration{
NewMigration("update reactions constraint", updateReactionConstraint),
// v160 -> v161
NewMigration("Add block on official review requests branch protection", addBlockOnOfficialReviewRequests),
// v161 -> v162
NewMigration("Add process field to task table", addProcessToTask),
}

// GetCurrentDBVersion returns the current db version
Expand Down
17 changes: 17 additions & 0 deletions models/migrations/v161.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright 2020 The Gitea Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Well, this file certainly needs to be renamed until this PR will be merged.

// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package migrations

import (
"xorm.io/xorm"
)

func addProcessToTask(x *xorm.Engine) error {
type Task struct {
Process string // process GUID and PID
}

return x.Sync2(new(Task))
}
12 changes: 12 additions & 0 deletions models/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type Task struct {
PayloadContent string `xorm:"TEXT"`
Errors string `xorm:"TEXT"` // if task failed, saved the error reason
Created timeutil.TimeStamp `xorm:"created"`
Process string // process GUID and PID
}

// LoadRepo loads repository of the task
Expand Down Expand Up @@ -114,6 +115,17 @@ func (task *Task) MigrateConfig() (*migration.MigrateOptions, error) {
return nil, fmt.Errorf("Task type is %s, not Migrate Repo", task.Type.Name())
}

// GUIDandPID return GUID and PID of a running task
func (task *Task) GUIDandPID() (guid int64, pid int64) {
Copy link
Member

Choose a reason for hiding this comment

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

I think simply returning 0, 0 is not always the best idea.
What if you extended the return to produce an additional error ((guid, pid int64, err error))?
Alternatively, you could set the error return value to -1, -1.
This is because it is guaranteed that no process will ever have a negative PID, while it could happen that a process has PID 0, although that is close to impossible for a process that is not the OS.

if task.Status != structs.TaskStatusRunning {
return 0, 0
}
if _, err := fmt.Sscanf(task.Process, "%d/%d", &guid, &pid); err != nil {
return 0, 0
}
return
Copy link
Member

Choose a reason for hiding this comment

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

Does that compile?
To me, this seems like a syntax error.
This whole function seems not finished.
Could it be that you intended to finish it another time?

Copy link
Member

Choose a reason for hiding this comment

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

If it compiles, I can understand why, but I wouldn't have dreamed up such a syntax ever…

Copy link
Member Author

Choose a reason for hiding this comment

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

}

// ErrTaskDoesNotExist represents a "TaskDoesNotExist" kind of error.
type ErrTaskDoesNotExist struct {
ID int64
Expand Down
11 changes: 11 additions & 0 deletions modules/process/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import (
"sort"
"sync"
"time"

gouuid "github.com/google/uuid"
)

// TODO: This packages still uses a singleton for the Manager.
Expand Down Expand Up @@ -44,18 +46,27 @@ type Manager struct {

counter int64
processes map[int64]*Process

// guid is a unique id for a gitea instance
guid int64
}

// GetManager returns a Manager and initializes one as singleton if there's none yet
func GetManager() *Manager {
if manager == nil {
manager = &Manager{
processes: make(map[int64]*Process),
guid: int64(gouuid.New().ID()),
}
}
return manager
}

// GUID return a unique id of a gitea instance
func (pm *Manager) GUID() int64 {
return pm.guid
Copy link
Member

Choose a reason for hiding this comment

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

The guid will be changed once the Gitea restarted?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

}

// Add a process to the ProcessManager and returns its PID.
func (pm *Manager) Add(description string, cancel context.CancelFunc) int64 {
pm.mutex.Lock()
Expand Down
3 changes: 2 additions & 1 deletion modules/task/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ func runMigrateTask(t *models.Task) (err error) {

t.StartTime = timeutil.TimeStampNow()
t.Status = structs.TaskStatusRunning
if err = t.UpdateCols("start_time", "status"); err != nil {
t.Process = fmt.Sprintf("%d/%d", process.GetManager().GUID(), pid)
if err = t.UpdateCols("start_time", "status", "process"); err != nil {
return
}

Expand Down
2 changes: 2 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,8 @@ migrate.migrate_items_options = Access Token is required to migrate additional i
migrated_from = Migrated from <a href="%[1]s">%[2]s</a>
migrated_from_fake = Migrated From %[1]s
migrate.migrate = Migrate From %s
migrate.cancelled = The migration has been cancelled.
migrate.task_not_found = The migration task for %s not found.
migrate.migrating = Migrating from <b>%s</b> ...
migrate.migrating_failed = Migrating from <b>%s</b> failed.
migrate.github.description = Migrating data from Github.com or Github Enterprise.
Expand Down
55 changes: 55 additions & 0 deletions routers/repo/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,22 @@
package repo

import (
"net/http"
"strings"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/auth"
"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/migrations"
"code.gitea.io/gitea/modules/process"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/task"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
repo_service "code.gitea.io/gitea/services/repository"
)

const (
Expand Down Expand Up @@ -188,3 +193,53 @@ func MigratePost(ctx *context.Context, form auth.MigrateRepoForm) {

handleMigrateError(ctx, ctxUser, err, "MigratePost", tpl, &form)
}

// CancelMigration cancels a running migration
func CancelMigration(ctx *context.Context) {
if !ctx.Repo.IsAdmin() {
ctx.Error(http.StatusNotFound)
return
}

if ctx.Repo.Repository.Status != models.RepositoryBeingMigrated {
ctx.Error(http.StatusConflict, "repo already migrated")
return
}

guid := process.GetManager().GUID()
task, err := models.GetMigratingTask(ctx.Repo.Repository.ID)
if err != nil {
ctx.InternalServerError(err)
return
}
if task.Status != structs.TaskStatusRunning {
task.Status = structs.TaskStatusStopped
task.EndTime = timeutil.TimeStampNow()
task.RepoID = 0

if err = task.UpdateCols("status", "repo_id", "end_time"); err != nil {
log.Error("Task UpdateCols failed: %v", err)
}

if err := repo_service.DeleteRepository(ctx.User, ctx.Repo.Repository); err != nil {
ctx.ServerError("DeleteRepository", err)
return
}

ctx.Flash.Success(ctx.Tr("repo.migrate.cancelled"))
ctx.Redirect(ctx.Repo.Owner.DashboardLink())
return
}

tGUID, tPID := task.GUIDandPID()
if guid == tGUID {
log.Trace("Migration [%s] cancel PID [%d] ", ctx.Repo.Repository.FullName(), tPID)
process.GetManager().Cancel(tPID)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be best to delete the repository here explicitly.
Otherwise, the repo might or might not exist in the database,
resulting in the user being unable to recreate a repo under this name until
either an admin runs a health check, or a doctor command is scheduled.

ctx.Flash.Success(ctx.Tr("repo.migrate.cancelled"))
ctx.Redirect(ctx.Repo.Owner.DashboardLink())
Copy link
Member

Choose a reason for hiding this comment

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

Is it even sensible to flash a message if the next action is to redirect back to the homepage?

return
}

ctx.Flash.Error(ctx.Tr("repo.migrate.task_not_found", ctx.Repo.Repository.FullName()))
Copy link
Member

Choose a reason for hiding this comment

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

The user will see a migrating task, but when click cancel, gitea will say it's not exist. This is still a problem.

Copy link
Member

Choose a reason for hiding this comment

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

Is it even sensible to flash a message if the next action is to redirect back to the homepage?

ctx.Redirect(ctx.Repo.Owner.DashboardLink())
}
1 change: 1 addition & 0 deletions routers/routes/macaron.go
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,7 @@ func RegisterMacaronRoutes(m *macaron.Macaron) {
m.Get("/:username/:reponame/releases/download/:vTag/:fileName", ignSignIn, context.RepoAssignment(), repo.MustBeNotEmpty, repo.RedirectDownload)

m.Group("/:username/:reponame", func() {
m.Post("/migrate/cancel", repo.CancelMigration)
m.Group("/settings", func() {
m.Combo("").Get(repo.Settings).
Post(bindIgnErr(auth.RepoSettingForm{}), repo.SettingsPost)
Expand Down
11 changes: 11 additions & 0 deletions templates/repo/migrate/migrating.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@
</div>
</div>
</div>
{{if .Permission.IsAdmin}}
<div class="ui divider"></div>
<div class="item center">
<form action="{{.Link}}/migrate/cancel" method="post">
{{.CsrfTokenHtml}}
<button class="ui button" data-do="delete">
<span class="item text">{{.i18n.Tr "cancel"}}</span>
</button>
</form>
</div>
{{end}}
</div>
</div>
</div>
Expand Down