From ab66bfff9186e45cd6cb58ed85ee49b5630a3692 Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Tue, 4 Jun 2024 23:00:56 -0500 Subject: [PATCH 1/2] fix: allow actions artifacts storage migration to complete succesfully (#31251) Change the copy to use `ActionsArtifact.StoragePath` instead of the `ArtifactPath`. Skip artifacts that are expired, and don't error if the file to copy does not exist. --- When trying to migrate actions artifact storage from local to MinIO, we encountered errors that prevented the process from completing successfully: * The migration tries to copy the files using the per-run `ArtifactPath`, instead of the unique `StoragePath`. * Artifacts that have been marked expired and had their files deleted would throw an error * Artifacts that are pending, but don't have a file uploaded yet will throw an error. This PR addresses these cases, and allow the process to complete successfully. (cherry picked from commit 8de8972baf5d82ff7b58ed77d78e8e1869e64eb5) (cherry picked from commit 4afbfd39467c558a06781c7cccf735df592a4bab) --- cmd/migrate_storage.go | 18 ++++++++++++++++-- release-notes/8.0.0/fix/4085.md | 1 + 2 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 release-notes/8.0.0/fix/4085.md diff --git a/cmd/migrate_storage.go b/cmd/migrate_storage.go index 1a8a37543d..69fdb90f33 100644 --- a/cmd/migrate_storage.go +++ b/cmd/migrate_storage.go @@ -5,7 +5,9 @@ package cmd import ( "context" + "errors" "fmt" + "io/fs" "strings" actions_model "code.gitea.io/gitea/models/actions" @@ -162,8 +164,20 @@ func migrateActionsLog(ctx context.Context, dstStorage storage.ObjectStorage) er func migrateActionsArtifacts(ctx context.Context, dstStorage storage.ObjectStorage) error { return db.Iterate(ctx, nil, func(ctx context.Context, artifact *actions_model.ActionArtifact) error { - _, err := storage.Copy(dstStorage, artifact.ArtifactPath, storage.ActionsArtifacts, artifact.ArtifactPath) - return err + if artifact.Status == int64(actions_model.ArtifactStatusExpired) { + return nil + } + + _, err := storage.Copy(dstStorage, artifact.StoragePath, storage.ActionsArtifacts, artifact.StoragePath) + if err != nil { + // ignore files that do not exist + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return err + } + + return nil }) } diff --git a/release-notes/8.0.0/fix/4085.md b/release-notes/8.0.0/fix/4085.md new file mode 100644 index 0000000000..6112b2f920 --- /dev/null +++ b/release-notes/8.0.0/fix/4085.md @@ -0,0 +1 @@ +- `forgejo migrate-storage --type actions-artifacts` always fails because it picks the wrong path From 43cd6e34e043a3a192fa7f97d25f52642fb96255 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 9 Jun 2024 15:49:22 +0200 Subject: [PATCH 2/2] tests(cmd): add coverage for migrateActionsArtifacts Also convert a comment into a warning in the logs when the deletion of an artifact cannot find the file in the destination storage. The case were an error happens while deleting the file is not covered as it would require to mock the storage.Copy function. (cherry picked from commit e75979440812832ae94bde9f37bd2047a1e388f0) --- cmd/migrate_storage.go | 2 +- cmd/migrate_storage_test.go | 75 +++++++++++++++++++++++++++++++++---- 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/cmd/migrate_storage.go b/cmd/migrate_storage.go index 69fdb90f33..3a69b555e0 100644 --- a/cmd/migrate_storage.go +++ b/cmd/migrate_storage.go @@ -170,8 +170,8 @@ func migrateActionsArtifacts(ctx context.Context, dstStorage storage.ObjectStora _, err := storage.Copy(dstStorage, artifact.StoragePath, storage.ActionsArtifacts, artifact.StoragePath) if err != nil { - // ignore files that do not exist if errors.Is(err, fs.ErrNotExist) { + log.Warn("ignored: actions artifact %s exists in the database but not in storage", artifact.StoragePath) return nil } return err diff --git a/cmd/migrate_storage_test.go b/cmd/migrate_storage_test.go index 5d8c867993..fa7067f97b 100644 --- a/cmd/migrate_storage_test.go +++ b/cmd/migrate_storage_test.go @@ -5,10 +5,12 @@ package cmd import ( "context" + "io" "os" "strings" "testing" + "code.gitea.io/gitea/models/actions" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/packages" "code.gitea.io/gitea/models/unittest" @@ -16,11 +18,28 @@ import ( packages_module "code.gitea.io/gitea/modules/packages" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/storage" + "code.gitea.io/gitea/modules/test" packages_service "code.gitea.io/gitea/services/packages" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func createLocalStorage(t *testing.T) (storage.ObjectStorage, string) { + t.Helper() + + p := t.TempDir() + + storage, err := storage.NewLocalStorage( + context.Background(), + &setting.Storage{ + Path: p, + }) + require.NoError(t, err) + + return storage, p +} + func TestMigratePackages(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) @@ -55,14 +74,7 @@ func TestMigratePackages(t *testing.T) { ctx := context.Background() - p := t.TempDir() - - dstStorage, err := storage.NewLocalStorage( - ctx, - &setting.Storage{ - Path: p, - }) - assert.NoError(t, err) + dstStorage, p := createLocalStorage(t) err = migratePackages(ctx, dstStorage) assert.NoError(t, err) @@ -73,3 +85,50 @@ func TestMigratePackages(t *testing.T) { assert.EqualValues(t, "01", entries[0].Name()) assert.EqualValues(t, "tmp", entries[1].Name()) } + +func TestMigrateActionsArtifacts(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + srcStorage, _ := createLocalStorage(t) + defer test.MockVariableValue(&storage.ActionsArtifacts, srcStorage)() + id := int64(0) + + addArtifact := func(storagePath string, status actions.ArtifactStatus) { + id++ + artifact := &actions.ActionArtifact{ + ID: id, + ArtifactName: storagePath, + StoragePath: storagePath, + Status: int64(status), + } + _, err := db.GetEngine(db.DefaultContext).Insert(artifact) + require.NoError(t, err) + srcStorage.Save(storagePath, strings.NewReader(storagePath), -1) + } + + exists := "/exists" + addArtifact(exists, actions.ArtifactStatusUploadConfirmed) + + expired := "/expired" + addArtifact(expired, actions.ArtifactStatusExpired) + + notFound := "/notfound" + addArtifact(notFound, actions.ArtifactStatusUploadConfirmed) + srcStorage.Delete(notFound) + + dstStorage, _ := createLocalStorage(t) + + assert.NoError(t, migrateActionsArtifacts(db.DefaultContext, dstStorage)) + + object, err := dstStorage.Open(exists) + assert.NoError(t, err) + buf, err := io.ReadAll(object) + require.NoError(t, err) + assert.Equal(t, exists, string(buf)) + + _, err = dstStorage.Stat(expired) + assert.Error(t, err) + + _, err = dstStorage.Stat(notFound) + assert.Error(t, err) +}