From 541eb3befd083bb796b08f6eccd2cadfa6f24f9d Mon Sep 17 00:00:00 2001 From: Amin Salarkia Date: Fri, 16 Aug 2024 15:56:16 +0200 Subject: [PATCH] feat(manifest-service): skip minor commits during cleanup (#1889) Ref: SRX-41DILA Releases that do not change any manifests are considered "minor". These releases are skipped during cleanup process. --- docs/minor-commits.md | 3 + pkg/db/db.go | 2 + .../cd-service/pkg/repository/repository.go | 1 + .../cd-service/pkg/repository/transformer.go | 63 ++++ .../pkg/repository/transformer_db_test.go | 296 ++++++++++++++++++ .../pkg/repository/transformer.go | 22 +- .../pkg/repository/transformer_test.go | 238 +++++++++++++- 7 files changed, 623 insertions(+), 2 deletions(-) create mode 100644 docs/minor-commits.md diff --git a/docs/minor-commits.md b/docs/minor-commits.md new file mode 100644 index 000000000..0ab678ada --- /dev/null +++ b/docs/minor-commits.md @@ -0,0 +1,3 @@ +# Minor Commits +Sometimes there are some commits that do not change any manifests in any environments at all. We consider these commits as "Minor". +Usually during the cleanup process kuberpult will keep some releases (20 by default, see `git.releaseVersionsLimit` in values.yaml) and remove all old releases other than that. During this process kuberpult will skip these "minor" releases, meaning that it will keep at least 20 releases, that are not minor. \ No newline at end of file diff --git a/pkg/db/db.go b/pkg/db/db.go index a7457043d..726083c5a 100644 --- a/pkg/db/db.go +++ b/pkg/db/db.go @@ -503,6 +503,7 @@ type DBReleaseMetaData struct { SourceMessage string DisplayVersion string UndeployVersion bool + IsMinor bool } type DBReleaseManifests struct { @@ -697,6 +698,7 @@ func (h *DBHandler) processReleaseRows(ctx context.Context, err error, rows *sql SourceMessage: "", DisplayVersion: "", UndeployVersion: false, + IsMinor: false, } err = json.Unmarshal(([]byte)(metadataStr), &metaData) if err != nil { diff --git a/services/cd-service/pkg/repository/repository.go b/services/cd-service/pkg/repository/repository.go index 238807998..c3d97cc64 100644 --- a/services/cd-service/pkg/repository/repository.go +++ b/services/cd-service/pkg/repository/repository.go @@ -2353,6 +2353,7 @@ func (s *State) WriteAllReleases(ctx context.Context, transaction *sql.Tx, app s SourceCommitId: repoRelease.SourceCommitId, SourceMessage: repoRelease.SourceMessage, DisplayVersion: repoRelease.DisplayVersion, + IsMinor: false, }, Deleted: false, } diff --git a/services/cd-service/pkg/repository/transformer.go b/services/cd-service/pkg/repository/transformer.go index 9a4537b38..1dc4614a9 100644 --- a/services/cd-service/pkg/repository/transformer.go +++ b/services/cd-service/pkg/repository/transformer.go @@ -635,6 +635,10 @@ func (c *CreateApplicationVersion) Transform( if len(prevRelease) > 0 { v = prevRelease[0].EslVersion } + isMinor, err := c.checkMinorFlags(ctx, transaction, state.DBHandler, version) + if err != nil { + return "", err + } release := db.DBReleaseWithMetaData{ EslVersion: 0, ReleaseNumber: version, @@ -648,6 +652,7 @@ func (c *CreateApplicationVersion) Transform( SourceMessage: c.SourceMessage, DisplayVersion: c.DisplayVersion, UndeployVersion: false, + IsMinor: isMinor, }, Created: time.Now(), Deleted: false, @@ -733,6 +738,63 @@ func (c *CreateApplicationVersion) Transform( return fmt.Sprintf("created version %d of %q", version, c.Application), nil } +func (c *CreateApplicationVersion) checkMinorFlags(ctx context.Context, transaction *sql.Tx, dbHandler *db.DBHandler, version uint64) (bool, error) { + allReleases, err := dbHandler.DBSelectAllReleasesOfApp(ctx, transaction, c.Application) + if err != nil { + return false, err + } + if allReleases == nil { + return false, err + } + releaseVersions := allReleases.Metadata.Releases + sort.Slice(releaseVersions, func(i, j int) bool { return releaseVersions[i] > releaseVersions[j] }) + nextVersion := int64(-1) + previousVersion := int64(-1) + for i := len(releaseVersions) - 1; i >= 0; i-- { + if releaseVersions[i] > int64(version) { + nextVersion = releaseVersions[i] + break + } + } + for i := 0; i < len(releaseVersions); i++ { + if releaseVersions[i] < int64(version) { + previousVersion = releaseVersions[i] + break + } + } + if nextVersion != -1 { + nextRelease, err := dbHandler.DBSelectReleaseByVersion(ctx, transaction, c.Application, uint64(nextVersion)) + if err != nil { + return false, err + } + nextRelease.Metadata.IsMinor = compareManifests(ctx, c.Manifests, nextRelease.Manifests.Manifests) + err = dbHandler.DBInsertRelease(ctx, transaction, *nextRelease, nextRelease.EslVersion) + if err != nil { + return false, err + } + } + if previousVersion == -1 { + return false, nil + } + previousRelease, err := dbHandler.DBSelectReleaseByVersion(ctx, transaction, c.Application, uint64(previousVersion)) + if err != nil { + return false, err + } + return compareManifests(ctx, c.Manifests, previousRelease.Manifests.Manifests), nil +} + +func compareManifests(ctx context.Context, firstManifests map[string]string, secondManifests map[string]string) bool { + if len(firstManifests) != len(secondManifests) { + return false + } + for key, value1 := range firstManifests { + if value2, exists := secondManifests[key]; !exists || value1 != value2 { + return false + } + } + return true +} + func getGenerator(ctx context.Context) uuid.GenerateUUIDs { gen, ok := ctx.Value(ctxMarkerGenerateUuidKey).(uuid.GenerateUUIDs) if !ok || gen == nil { @@ -1141,6 +1203,7 @@ func (c *CreateUndeployApplicationVersion) Transform( SourceMessage: "", DisplayVersion: "", UndeployVersion: true, + IsMinor: false, }, Created: time.Now(), Deleted: false, diff --git a/services/cd-service/pkg/repository/transformer_db_test.go b/services/cd-service/pkg/repository/transformer_db_test.go index c1c387e6d..4754d39c6 100644 --- a/services/cd-service/pkg/repository/transformer_db_test.go +++ b/services/cd-service/pkg/repository/transformer_db_test.go @@ -782,6 +782,302 @@ func TestCreateApplicationVersionDB(t *testing.T) { } } +func TestMinorFlag(t *testing.T) { + appName := "app" + tcs := []struct { + Name string + Transformers []Transformer + ExpectedMinors []uint64 + ExpectedMajors []uint64 + }{ + { + Name: "No previous or next releases", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + }, + ExpectedMinors: []uint64{}, + ExpectedMajors: []uint64{10}, + }, + { + Name: "No next Release, Previous Releases manifest equals current releases", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 11, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + }, + ExpectedMinors: []uint64{11}, + ExpectedMajors: []uint64{10}, + }, + { + Name: "No next Release, Previous Releases Manifest does not equal current's", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 11, + Manifests: map[string]string{ + envAcceptance: "manifest2", + }, + }, + }, + ExpectedMinors: []uint64{}, + ExpectedMajors: []uint64{10, 11}, + }, + { + Name: "No prev Release, next Releases Manifest equals current's", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 11, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + }, + ExpectedMinors: []uint64{11}, + ExpectedMajors: []uint64{10}, + }, + { + Name: "No prev Release, next Releases Manifest does not equal current's", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 11, + Manifests: map[string]string{ + envAcceptance: "manifest2", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + }, + ExpectedMinors: []uint64{}, + ExpectedMajors: []uint64{10, 11}, + }, + { + Name: "prev, next, and current are not equal", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 12, + Manifests: map[string]string{ + envAcceptance: "manifest3", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 11, + Manifests: map[string]string{ + envAcceptance: "manifest2", + }, + }, + }, + ExpectedMinors: []uint64{}, + ExpectedMajors: []uint64{10, 11, 12}, + }, + { + Name: "prev and current are equal but not next", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 12, + Manifests: map[string]string{ + envAcceptance: "manifest1", + "new env": "new manifest", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 11, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + }, + ExpectedMinors: []uint64{11}, + ExpectedMajors: []uint64{10, 12}, + }, + { + Name: "prev and next are equal but not current", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 12, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 11, + Manifests: map[string]string{ + envAcceptance: "manifest2", + }, + }, + }, + ExpectedMinors: []uint64{}, + ExpectedMajors: []uint64{10, 11, 12}, + }, + { + Name: "current and next are equal but not prev", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest2", + "new env": "new manifest", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 12, + Manifests: map[string]string{ + envAcceptance: "manifest2", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 11, + Manifests: map[string]string{ + envAcceptance: "manifest2", + }, + }, + }, + ExpectedMinors: []uint64{12}, + ExpectedMajors: []uint64{10, 11}, + }, + { + Name: "all equal", + Transformers: []Transformer{ + &CreateApplicationVersion{ + Application: appName, + Version: 10, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 12, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + &CreateApplicationVersion{ + Application: appName, + Version: 11, + Manifests: map[string]string{ + envAcceptance: "manifest1", + }, + }, + }, + ExpectedMinors: []uint64{11, 12}, + ExpectedMajors: []uint64{10}, + }, + } + + for _, tc := range tcs { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + + ctxWithTime := time.WithTimeNow(testutil.MakeTestContext(), timeNowOld) + repo := SetupRepositoryTestWithDB(t) + err3 := repo.State().DBHandler.WithTransactionR(ctxWithTime, 0, false, func(ctx context.Context, transaction *sql.Tx) error { + _, state, _, err := repo.ApplyTransformersInternal(ctx, transaction, &CreateEnvironment{ + Environment: "acceptance", + Config: config.EnvironmentConfig{Upstream: &config.EnvironmentConfigUpstream{Environment: envAcceptance, Latest: false}}, + }) + if err != nil { + return err + } + _, state, _, err = repo.ApplyTransformersInternal(ctx, transaction, tc.Transformers...) + if err != nil { + return err + } + for _, minorVersion := range tc.ExpectedMinors { + release, err := state.DBHandler.DBSelectReleaseByVersion(ctxWithTime, transaction, appName, minorVersion) + if err != nil { + return err + } + if !release.Metadata.IsMinor { + t.Errorf("Expected release %d to be minor but its major", minorVersion) + } + } + for _, majorVersion := range tc.ExpectedMajors { + release, err := state.DBHandler.DBSelectReleaseByVersion(ctxWithTime, transaction, appName, majorVersion) + if err != nil { + return err + } + if release.Metadata.IsMinor { + t.Errorf("Expected release %d to be major but its minor", majorVersion) + } + } + return nil + }) + if err3 != nil { + t.Fatalf("expected no error, got %v", err3) + } + }) + } +} + func TestDeleteQueueApplicationVersion(t *testing.T) { tcs := []struct { Name string diff --git a/services/manifest-repo-export-service/pkg/repository/transformer.go b/services/manifest-repo-export-service/pkg/repository/transformer.go index 4d6cb2e86..b7f07b905 100644 --- a/services/manifest-repo-export-service/pkg/repository/transformer.go +++ b/services/manifest-repo-export-service/pkg/repository/transformer.go @@ -1069,7 +1069,27 @@ func findOldApplicationVersions(ctx context.Context, transaction *sql.Tx, state if positionOfOldestVersion < (int(state.ReleaseVersionsLimit) - 1) { return nil, nil } - return versions[0 : positionOfOldestVersion-(int(state.ReleaseVersionsLimit)-1)], err + indexToKeep := positionOfOldestVersion - 1 + majorsCount := 0 + for ; indexToKeep >= 0; indexToKeep-- { + release, err := state.DBHandler.DBSelectReleaseByVersion(ctx, transaction, name, versions[indexToKeep]) + if err != nil { + return nil, err + } + if release == nil { + majorsCount += 1 + logger.FromContext(ctx).Warn("Release not found in database") + } else if !release.Metadata.IsMinor { + majorsCount += 1 + } + if majorsCount >= int(state.ReleaseVersionsLimit) { + break + } + } + if indexToKeep < 0 { + return nil, nil + } + return versions[0:indexToKeep], nil } func GetLastRelease(fs billy.Filesystem, application string) (uint64, error) { diff --git a/services/manifest-repo-export-service/pkg/repository/transformer_test.go b/services/manifest-repo-export-service/pkg/repository/transformer_test.go index 7e36be968..9da6ccfae 100644 --- a/services/manifest-repo-export-service/pkg/repository/transformer_test.go +++ b/services/manifest-repo-export-service/pkg/repository/transformer_test.go @@ -889,13 +889,14 @@ func TestCleanupOldApplicationVersions(t *testing.T) { tcs := []struct { Name string Transformers []Transformer + MinorRelease uint64 ExpectedError error ExpectedFile []*FilenameAndData ExpectedAuthor *map[string]string ExpectedDeletedFile string }{ { - Name: "CleanupOldApplicationVersions", //ReleaseLimit is 2 + Name: "CleanupOldApplicationVersion without deleting any", //ReleaseLimit is 2 Transformers: []Transformer{ &CreateApplicationVersion{ Authentication: Authentication{}, @@ -981,6 +982,228 @@ func TestCleanupOldApplicationVersions(t *testing.T) { }, ExpectedAuthor: &map[string]string{"Name": authorName, "Email": authorEmail}, }, + { + Name: "CleanupOldApplicationVersions deleting one", //ReleaseLimit is 2 + Transformers: []Transformer{ + &CreateApplicationVersion{ + Authentication: Authentication{}, + Version: 1, + Application: appName, + Manifests: map[string]string{ + envAcceptance: "mani-1-acc", + envDev: "mani-1-dev", + }, + SourceCommitId: "123456789", + SourceAuthor: "", + SourceMessage: "", + Team: "team-123", + DisplayVersion: "", + WriteCommitData: false, + PreviousCommit: "", + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 1, + }, + &CreateApplicationVersion{ + Authentication: Authentication{}, + Version: 2, + Application: appName, + Manifests: map[string]string{ + envAcceptance: "mani-1-acc", + envDev: "mani-1-dev", + }, + SourceCommitId: "abcdef", + SourceAuthor: "", + SourceMessage: "", + Team: "team-123", + DisplayVersion: "", + WriteCommitData: false, + PreviousCommit: "", + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 2, + }, + &CreateApplicationVersion{ + Authentication: Authentication{}, + Version: 3, + Application: appName, + Manifests: map[string]string{ + envAcceptance: "mani-1-acc", + envDev: "mani-1-dev", + }, + SourceCommitId: "123456789abcdef", + SourceAuthor: "", + SourceMessage: "", + Team: "team-123", + DisplayVersion: "", + WriteCommitData: false, + PreviousCommit: "", + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 3, + }, + &CreateApplicationVersion{ + Authentication: Authentication{}, + Version: 4, + Application: appName, + Manifests: map[string]string{ + envAcceptance: "mani-1-acc", + envDev: "mani-1-dev", + }, + SourceCommitId: "123456789abcdef", + SourceAuthor: "", + SourceMessage: "", + Team: "team-123", + DisplayVersion: "", + WriteCommitData: false, + PreviousCommit: "", + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 4, + }, + &CleanupOldApplicationVersions{ + Application: appName, + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 5, + }, + }, + ExpectedFile: []*FilenameAndData{ + { + path: "/applications/" + appName + "/releases/3/source_commit_id", + fileData: []byte("123456789abcdef"), + }, + { + path: "/applications/" + appName + "/releases/2/source_commit_id", + fileData: []byte("abcdef"), + }, + }, + ExpectedAuthor: &map[string]string{"Name": authorName, "Email": authorEmail}, + ExpectedDeletedFile: "/applications/" + appName + "/releases/1/source_commit_id", + }, + { + Name: "CleanupOldApplicationVersions with a minor release", //ReleaseLimit is 2 + Transformers: []Transformer{ + &CreateApplicationVersion{ + Authentication: Authentication{}, + Version: 1, + Application: appName, + Manifests: map[string]string{ + envAcceptance: "mani-1-acc", + envDev: "mani-1-dev", + }, + SourceCommitId: "123456789", + SourceAuthor: "", + SourceMessage: "", + Team: "team-123", + DisplayVersion: "", + WriteCommitData: false, + PreviousCommit: "", + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 1, + }, + &CreateApplicationVersion{ + Authentication: Authentication{}, + Version: 2, + Application: appName, + Manifests: map[string]string{ + envAcceptance: "mani-1-acc", + envDev: "mani-1-dev", + }, + SourceCommitId: "abcdef", + SourceAuthor: "", + SourceMessage: "", + Team: "team-123", + DisplayVersion: "", + WriteCommitData: false, + PreviousCommit: "", + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 2, + }, + &CreateApplicationVersion{ + Authentication: Authentication{}, + Version: 3, + Application: appName, + Manifests: map[string]string{ + envAcceptance: "mani-1-acc", + envDev: "mani-1-dev", + }, + SourceCommitId: "123456789abcdef", + SourceAuthor: "", + SourceMessage: "", + Team: "team-123", + DisplayVersion: "", + WriteCommitData: false, + PreviousCommit: "", + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 3, + }, + &CreateApplicationVersion{ + Authentication: Authentication{}, + Version: 4, + Application: appName, + Manifests: map[string]string{ + envAcceptance: "mani-1-acc", + envDev: "mani-1-dev", + }, + SourceCommitId: "123456789abcdef", + SourceAuthor: "", + SourceMessage: "", + Team: "team-123", + DisplayVersion: "", + WriteCommitData: false, + PreviousCommit: "", + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 4, + }, + &CleanupOldApplicationVersions{ + Application: appName, + TransformerMetadata: TransformerMetadata{ + AuthorName: authorName, + AuthorEmail: authorEmail, + }, + TransformerEslVersion: 5, + }, + }, + MinorRelease: 3, + ExpectedFile: []*FilenameAndData{ + { + path: "/applications/" + appName + "/releases/3/source_commit_id", + fileData: []byte("123456789abcdef"), + }, + { + path: "/applications/" + appName + "/releases/2/source_commit_id", + fileData: []byte("abcdef"), + }, + { + path: "/applications/" + appName + "/releases/1/source_commit_id", + fileData: []byte("123456789"), + }, + }, + ExpectedAuthor: &map[string]string{"Name": authorName, "Email": authorEmail}, + }, } for _, tc := range tcs { tc := tc @@ -1017,6 +1240,19 @@ func TestCleanupOldApplicationVersions(t *testing.T) { Metadata: db.DBReleaseMetaData{}, }, db.InitialEslVersion) + if tc.MinorRelease != 0 { + err = dbHandler.DBInsertRelease(ctx, transaction, db.DBReleaseWithMetaData{ + EslVersion: 1, + ReleaseNumber: tc.MinorRelease, + Created: time.Time{}, + App: appName, + Manifests: db.DBReleaseManifests{}, + Metadata: db.DBReleaseMetaData{ + IsMinor: true, + }, + }, 1) + } + if err != nil { return err }