Skip to content

Commit a7fa9ec

Browse files
dkropachevdkropachev
committed
fix(backup): bugs fixes
1 parent ad3d94f commit a7fa9ec

File tree

5 files changed

+65
-36
lines changed

5 files changed

+65
-36
lines changed

‎pkg/restapi/backup.go

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ func (h backupHandler) purge(w http.ResponseWriter, r *http.Request) {
217217
respondError(w, r, errors.Wrap(err, "manual purge snapshots"))
218218
return
219219
}
220+
220221
render.Respond(w, r, ConvertManifestsToListItems(deleted))
221222
}
222223

@@ -232,13 +233,28 @@ func ConvertManifestsToListItems(deleted backupspec.Manifests) backup.ListItems
232233
Units: nil,
233234
SnapshotInfo: []backup.SnapshotInfo{{
234235
SnapshotTag: manifest.SnapshotTag,
236+
Nodes: 1,
235237
}},
236238
}
237239
out = append(out, &item)
238240
continue
239241
}
240-
snInfo := backup.SnapshotInfo{SnapshotTag: manifest.SnapshotTag}
241-
out[idx].SnapshotInfo = append(out[idx].SnapshotInfo, snInfo)
242+
243+
// Found SnapshotInfo with same snapshotTag.
244+
found := false
245+
for sIdx := range out[idx].SnapshotInfo {
246+
if out[idx].SnapshotInfo[sIdx].SnapshotTag == manifest.SnapshotTag {
247+
out[idx].SnapshotInfo[sIdx].Nodes++
248+
found = true
249+
break
250+
}
251+
}
252+
253+
if !found {
254+
// If not found - creates new one SnapshotInfo.
255+
snInfo := backup.SnapshotInfo{SnapshotTag: manifest.SnapshotTag, Nodes: 1}
256+
out[idx].SnapshotInfo = append(out[idx].SnapshotInfo, snInfo)
257+
}
242258
}
243259
return out
244260
}
@@ -247,7 +263,7 @@ func (h backupHandler) dryRunToCtx(next http.Handler) http.Handler {
247263
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
248264
// Read dry-run tag from the request
249265
dryRun := false
250-
if v := r.FormValue("dry_run"); v != "" {
266+
if v := r.FormValue("dry_run"); v == "true" {
251267
dryRun = true
252268
}
253269

@@ -260,7 +276,7 @@ func (h backupHandler) dryRunToCtx(next http.Handler) http.Handler {
260276
func (h backupHandler) getDryRunFromCtx(r *http.Request) bool {
261277
v, ok := r.Context().Value(ctxDryRun).(bool)
262278
if !ok {
263-
panic("missing filter in context")
279+
panic("missing dryRun in context")
264280
}
265281
return v
266282
}

‎pkg/service/backup/backupspec/manifest.go

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package backupspec
55
import (
66
"compress/gzip"
77
"encoding/json"
8+
"fmt"
89
"io"
910
"os"
1011
"path"
@@ -13,10 +14,12 @@ import (
1314

1415
jsoniter "github.com/json-iterator/go"
1516
"github.com/pkg/errors"
17+
"go.uber.org/multierr"
18+
1619
"github.com/scylladb/scylla-manager/v3/pkg/util/inexlist/ksfilter"
1720
"github.com/scylladb/scylla-manager/v3/pkg/util/pathparser"
21+
"github.com/scylladb/scylla-manager/v3/pkg/util/slice"
1822
"github.com/scylladb/scylla-manager/v3/pkg/util/uuid"
19-
"go.uber.org/multierr"
2023
)
2124

2225
// ManifestInfo represents manifest on remote location.
@@ -33,20 +36,33 @@ type ManifestInfo struct {
3336
// Manifests is a slice of ManifestInfo.
3437
type Manifests []*ManifestInfo
3538

36-
// Len returns array len.
39+
// Len returns slice len.
3740
func (l Manifests) Len() int {
3841
return len(l)
3942
}
4043

41-
// GetBySnapshot returns map of ManifestInfo where key is SnapshotTag.
42-
func (l Manifests) GetBySnapshot() map[string]*ManifestInfo {
43-
out := make(map[string]*ManifestInfo)
44+
// Str returns a string contains all ManifestInfo separated by a newline.
45+
func (l Manifests) Str() string {
46+
if l.Len() == 0 {
47+
return ""
48+
}
49+
out := ""
4450
for idx := range l {
45-
out[l[idx].SnapshotTag] = l[idx]
51+
out += fmt.Sprintf("\n%+v", l[idx])
4652
}
53+
out += "\n"
4754
return out
4855
}
4956

57+
// GetSnapshots returns slice unique SnapshotTag.
58+
func (l Manifests) GetSnapshots() []string {
59+
out := make([]string, l.Len())
60+
for idx := range l {
61+
out[idx] = l[idx].SnapshotTag
62+
}
63+
return slice.UniqueNP(out)
64+
}
65+
5066
// Path returns path to the file that manifest points to.
5167
func (m *ManifestInfo) Path() string {
5268
f := RemoteManifestFile(m.ClusterID, m.TaskID, m.SnapshotTag, m.DC, m.NodeID)

‎pkg/service/backup/model.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@ import (
1010
"strconv"
1111
"time"
1212

13-
"github.com/scylladb/scylla-manager/v3/pkg/service/scheduler"
14-
1513
"github.com/gocql/gocql"
1614
"github.com/pkg/errors"
1715
"github.com/scylladb/go-set/strset"
@@ -20,6 +18,7 @@ import (
2018

2119
"github.com/scylladb/scylla-manager/v3/pkg/scyllaclient"
2220
. "github.com/scylladb/scylla-manager/v3/pkg/service/backup/backupspec"
21+
"github.com/scylladb/scylla-manager/v3/pkg/service/scheduler"
2322
"github.com/scylladb/scylla-manager/v3/pkg/util/uuid"
2423
)
2524

‎pkg/service/backup/purger_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ func TestStaleTags(t *testing.T) {
100100
"sm_19700101000040UTC",
101101
deletedByRetentionTag,
102102
}
103-
manifests.GetBySnapshot()
103+
104104
if diff := cmp.Diff(tags.List(), expected, cmpopts.SortSlices(func(a, b string) bool { return a < b })); diff != "" {
105105
t.Fatalf("staleTags() = %s, diff:\n%s", tags.List(), diff)
106106
}

‎pkg/service/backup/service.go

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ import (
1313
"time"
1414

1515
"github.com/pkg/errors"
16+
"go.uber.org/atomic"
17+
1618
"github.com/scylladb/go-log"
1719
"github.com/scylladb/go-set/strset"
1820
"github.com/scylladb/gocqlx/v2"
@@ -30,7 +32,6 @@ import (
3032
"github.com/scylladb/scylla-manager/v3/pkg/util/parallel"
3133
"github.com/scylladb/scylla-manager/v3/pkg/util/timeutc"
3234
"github.com/scylladb/scylla-manager/v3/pkg/util/uuid"
33-
"go.uber.org/atomic"
3435
)
3536

3637
var (
@@ -1190,24 +1191,23 @@ func (s *Service) DeleteSnapshot(ctx context.Context, clusterID uuid.UUID, locat
11901191
//
11911192
// It returns deleted Manifests. if dryRun is true nothing is deleted.
11921193
func (s *Service) PurgeBackups(ctx context.Context, clusterID uuid.UUID, locations Locations, retentionMap RetentionMap, dryRun bool) (deleted Manifests, err error) {
1193-
if locations.Len() == 0 {
1194-
return nil, errorNoLocation
1195-
}
1196-
locations = locations.Unique()
1197-
logger := s.logger
1194+
logger := s.logger.Named("PurgeBackups")
11981195
if dryRun {
11991196
logger = log.NopLogger
12001197
}
1201-
1202-
logger.Info(ctx, "Purging stale snapshots...")
1203-
1204-
defer func(start time.Time) {
1198+
logger.Debug(ctx, "Start", "cluster", clusterID, "locations", locations, "retentionMap", retentionMap, "dryRun", dryRun)
1199+
start := timeutc.Now()
1200+
defer func() {
12051201
if err != nil {
1206-
logger.Error(ctx, "Manual purging stale snapshots failed see exact errors above", "duration", timeutc.Since(start))
1207-
} else {
1208-
logger.Info(ctx, "Done manual purging stale snapshots", "duration", timeutc.Since(start))
1202+
logger.Error(ctx, "Got an error", "error", err, "duration", timeutc.Since(start))
12091203
}
1210-
}(timeutc.Now())
1204+
logger.Debug(ctx, "Done", "deleted manifests", deleted, "duration", timeutc.Since(start))
1205+
}()
1206+
1207+
if locations.Len() == 0 {
1208+
return nil, errorNoLocation
1209+
}
1210+
locations = locations.Unique()
12111211

12121212
deleted = make(Manifests, 0)
12131213

@@ -1229,11 +1229,11 @@ func (s *Service) PurgeBackups(ctx context.Context, clusterID uuid.UUID, locatio
12291229
}
12301230

12311231
// List manifests in all locations
1232-
manifests, err := listManifestsInAllLocations(ctx, client, hosts, clusterID)
1232+
var manifests Manifests
1233+
manifests, err = listManifestsInAllLocations(ctx, client, hosts, clusterID)
12331234
if err != nil {
12341235
return nil, errors.Wrap(err, "list manifests")
12351236
}
1236-
12371237
// Get a list of stale tags
12381238
tags, oldest, err := staleTags(manifests, retentionMap, true)
12391239
if err != nil {
@@ -1262,16 +1262,17 @@ func (s *Service) PurgeBackups(ctx context.Context, clusterID uuid.UUID, locatio
12621262
p := newPurger(client, hostIP, logg)
12631263

12641264
p.OnDelete = func(total, success int) {
1265-
if !dryRun {
1266-
s.metrics.Backup.SetPurgeFiles(clusterID, hostIP, total, success)
1265+
if dryRun {
1266+
return
12671267
}
1268+
s.metrics.Backup.SetPurgeFiles(clusterID, hostIP, total, success)
12681269
}
12691270

12701271
logg.Info(ctx, "Purging stale snapshots of node from host")
12711272
defer logg.Info(ctx, "Done purging stale snapshots of node from host")
12721273

12731274
var del Manifests
1274-
del, err = p.PurgeSnapshotTags(ctx, manifests, tags, oldest, dryRun)
1275+
del, err = p.PurgeSnapshotTags(ctx, m, tags, oldest, dryRun)
12751276

12761277
if err != nil {
12771278
return err
@@ -1286,9 +1287,7 @@ func (s *Service) PurgeBackups(ctx context.Context, clusterID uuid.UUID, locatio
12861287
if dryRun {
12871288
return nil
12881289
}
1289-
1290-
for idx := range del {
1291-
snapshot := del[idx].SnapshotTag
1290+
for _, snapshot := range del.GetSnapshots() {
12921291
if err = client.DeleteSnapshot(ctx, hostIP, snapshot); err != nil {
12931292
logg.Error(ctx, "Failed to delete uploaded snapshot",
12941293
"host", hostIP,
@@ -1306,6 +1305,5 @@ func (s *Service) PurgeBackups(ctx context.Context, clusterID uuid.UUID, locatio
13061305
}
13071306

13081307
parallel.RunMap[string, Manifests](nodeIDManifests, runFunc, nil, 10)
1309-
13101308
return deleted, err
13111309
}

0 commit comments

Comments
 (0)