Implement backup purge command to purge orphaned snapshots #3520
Conversation
dkropachev
force-pushed
the
dk/add-restapi_pugre
branch
3 times, most recently
from
a7fa9ec to
7542cbf
Compare
dkropachev
force-pushed
the
dk/add-restapi_pugre
branch
from
7542cbf to
05c1dba
Compare
dkropachev
requested review from
karol-kokoszka and
Michal-Leszczynski
as code owners
dkropachev
changed the title
| for idx := range l { | ||
| if l[idx] == location { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
Why iterate over idx instead of the actual value _, loc?
Also, comparing locations by all fields might be misleading, as one of those fields is DC.
I think that we would like to treat locations with the same provider and path as the same regardless of their dc assignment.
| for idx := range l { | ||
| if !out.Contains(l[idx]) { | ||
| out = append(out, l[idx]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why iterate over idx instead of the actual value _, loc?
| for idx := range l { | ||
| if l[idx].DC != "" { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
Why iterate over idx instead of the actual value _, loc?
pkg/service/backup/model.go
Outdated
| _, have := r[taskID] | ||
| return have |
There was a problem hiding this comment.
Isn't it more idiomatic to call this variable ok?
pkg/service/backup/model.go
Outdated
| func (r RetentionMap) GetPolicy(taskID uuid.UUID, onMissing *RetentionPolicy) *RetentionPolicy { | ||
| if policy, ok := r[taskID]; ok { | ||
| return &policy | ||
| } | ||
| return onMissing | ||
| } |
There was a problem hiding this comment.
Why would it return a pointer?
pkg/restapi/backup_test.go
Outdated
| err := assertJsonBody(w, golden) | ||
| if err != nil { |
There was a problem hiding this comment.
Maybe combine those two into a single line?
pkg/service/backup/list.go
Outdated
| for _, hi := range hosts { | ||
| if _, ok := locations[hi.Location]; ok { | ||
| for idx := range hosts { | ||
| if _, ok := locations[hosts[idx].Location]; ok { | ||
| continue | ||
| } | ||
| locations[hi.Location] = struct{}{} | ||
| locations[hosts[idx].Location] = struct{}{} | ||
|
|
||
| lm, err := listManifests(ctx, client, hi.IP, hi.Location, clusterID) | ||
| lm, err := listManifests(ctx, client, hosts[idx].IP, hosts[idx].Location, clusterID) |
There was a problem hiding this comment.
What is the motivation behind this change?
pkg/service/backup/parallel.go
Outdated
| for _, h := range hosts { | ||
| dc := h.DC | ||
| for idx := range hosts { | ||
| dc := hosts[idx].DC |
There was a problem hiding this comment.
What is the motivation behind this change?
pkg/service/backup/validation.go
Outdated
| for _, h := range hosts { | ||
| if h.ID == nodeID { | ||
| return h.IP | ||
| for idx := range hosts { | ||
| if hosts[idx].ID == nodeID { | ||
| return hosts[idx].IP |
There was a problem hiding this comment.
What is the motivation behind this change?
pkg/service/backup/worker_schema.go
Outdated
| for _, hi := range hosts { | ||
| locations[hi.Location.String()] = hi | ||
| for idx := range hosts { | ||
| locations[hosts[idx].Location.String()] = hosts[idx] | ||
| } | ||
| hostPerLocation := make([]hostInfo, 0, len(locations)) | ||
| for _, hi := range locations { | ||
| hostPerLocation = append(hostPerLocation, hi) | ||
| for key := range locations { | ||
| hostPerLocation = append(hostPerLocation, locations[key]) |
There was a problem hiding this comment.
What is the motivation behind this change?
dkropachev
force-pushed
the
dk/add-restapi_pugre
branch
from
05c1dba to
9f504cd
Compare
dkropachev
force-pushed
the
dk/add-restapi_pugre
branch
from
9f504cd to
c25efc5
Compare
|
@dkropachev are all the @Michal-Leszczynski addressed ? I don't see any responses to Michals questions. Please address the comments and ping for re-review from Michal, otherwise this PR will get stalled. |
|
@karol-kokoszka , not yet, I will complete it this week |
dkropachev
force-pushed
the
dk/add-restapi_pugre
branch
from
c25efc5 to
8b42f3e
Compare
Closes #3441
There could be the case when backup task is deleted, but snapshots are left on the location.
So, we need to have a command that deletes these stale snapshots