AWS: Ensure that the order returned by ListObjects is consistent #999
Conversation
|
@bashofmann could you provide an example of when you ran into an error? s3 doesn't have real folders - they exist because of the |
|
At least in our quobyte s3, if you have these objects: and you try to delete the first entry, you get: This is also visible in the ark logs where it tries to delete |
|
Ark should not be trying to delete a pseudo-directory ( |
|
Quobyte definitely returns this and I think in quobyte this pseudo-directory is actually a real key that also needs to be deleted. If I have the entries from above: and I delete everything but the pseudo-directory the pseudo-directory is still listed as an existing key and must be explicitly deleted |
|
Ok, so this sounds like Quobyte is not conforming to how s3 normally functions. Can you try running this and let us know the output? package main
import (
"fmt"
"log"
"os"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/s3"
)
func main() {
if len(os.Args) != 3 {
log.Fatalf("usage: %s BUCKET PREFIX\n", os.Args[0])
}
bucket := os.Args[1]
prefix := os.Args[2]
s, err := session.NewSession()
if err != nil {
log.Fatalf("error creating aws session/client: %v\n", err)
}
req := &s3.ListObjectsV2Input{
Bucket: &bucket,
Prefix: &prefix,
}
var ret []string
c := s3.New(s)
err = c.ListObjectsV2Pages(req, func(page *s3.ListObjectsV2Output, lastPage bool) bool {
for _, obj := range page.Contents {
ret = append(ret, *obj.Key)
}
return !lastPage
})
if err != nil {
log.Fatalf("error listing: %v\n", err)
}
for _, s := range ret {
fmt.Println(s)
}
}
|
|
(against a backup "folder" where the files still exist) |
|
|
Ok. I think a better fix here would be to adjust the aws objectstore listobjects method to omit keys that end in |
|
But with Quobyte this |
|
Ok, that makes sense. Just need some more clarifying details:
We'll eventually want a unit test, but this file isn't set up to do unit testing easily. I can extract some changes I made from #629 to make it possible to unit test this, but that can be after this merges. |
|
1: Not really unfortunately, even though s3cmd says it deleted "the folder", it's still there: 2: I'll add a comment in a few minutes. |
bashofmann
force-pushed
the
fix-backup-file-deletion-order
branch
from
de12b4f
to
6b9a7b7
Compare
|
LGTM. Let's get 1 more 👍 from another reviewer, and then you can squash & we'll merge. @wwitzel3 ptal |
|
👍 from me |
When a backup is deleted, the delete method uses ListObjects to get a list of files it needs to delete in s3. Different s3 implementations may return the object lists in different, even non-deterministic orders, which can result in the deletion not working because ark tries to delete a non empty folder before it tries to delete the files in the folder. Signed-off-by: Bastian Hofmann <bashofmann@gmail.com>
bashofmann
force-pushed
the
fix-backup-file-deletion-order
branch
from
6b9a7b7
to
8bbfc53
Compare
|
Commits are squashed. |
|
Thanks @bashofmann! |
|
@wwitzel3 Is there a backport PR for this yet? |
|
Backport PR #1028 |
When a backup is deleted, the delete method uses ListObjects to get a list of
files it needs to delete in s3. Different s3 implementations may return
the object lists in different, even non-deterministic orders, which can
result in the deletion not working because ark tries to delete a non empty folder
before it tries to delete the files in the folder.
Signed-off-by: Bastian Hofmann bashofmann@gmail.com