x-if-delete-at is bad

[gholt]

I've been meaning to record and fix this bug for quite some time; at least it's recorded now.

When I implemented expiring objects I screwed up and added the x-if-delete-at conditional for object deletes. This is wrong due to eventual consistency possibilities. Though the possibility of an issue is small, it does exist.

Imagine I put an object with the timestamp 1369160000.0000000 and I set it's x-delete-at value to 1369160010.0000000. This put succeeds on all three replicas.

Just afterwards I overwrite the object again with the timestamp 1369160001.0000000 and no x-delete-at value. This put succeeds, but only on two nodes.

Then, the expirer comes along later (and no replication has occurred yet to stabilize the out of date copy) and issues the delete. The delete fails on the two replicas with newer copies (and no x-delete-at value, so not matching) but succeeds on the out of date replica since it does still match. On the one replica where it succeeded it would make a new tombstone, say with the timestamp of 1369160011.0000000.

When replication kicks in it will replicate the newest action, the tombstone, over the newer actual objects.

The best fix I can imagine is dropping the x-if-delete-at conditional delete, because it's wrong, and instead making the expirer do a head on all replicas, check the x-delete-at headers itself, and issue the delete only if all replicas responded and matched. There are still race conditions that can arise, but they are quite a bit smaller.

Of course, I'm open to an even better solution, and I'm not sure when I'll actually get around to implementing this.

[clayg (clay-gerrard)]

I must be confused, I'll double check myself. But before you get to far into it... what about just issuing the DELETE with an X-Timestamp set to the expiry time?

[clayg (clay-gerrard)]

I guess i was wrong, reading it now it seems to work like you described. Not sure if dropping a tombstone at the x-delete-at time instead of when the request is issued solves the issue you described in full or not.

[Zhou Yuan (yuan-zhou)]

another solution might be: we could head the newest replica only({newest: t}), if its x-delete-at header is matched, then we're safe to issue the delete requests, if mismatch then we may need to break.

The cases of mismatching header should be:
1. newest x-delete-at header is empty
when heading on the expiring replicas, probably we might not get the x-delete-at header because:
a. the response might be a 404 error even if the object still exits but expired
b. the newest copy is updated and x-delete-at header is deleted

in case a, the newest copy is also expired, so it should be safe to delete,
in case b, we need to stop and wait for the replicator to update our self.

2. newest x-delete-at != local x-delete-at
In this case we need to stop and wait for the replicator

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/34095

[John Dickinson (notmyname)]

This needs to be re-evalauted and checked. Perhaps the old patch can be resurrected, if that's needed.

[Launchpad Janitor (janitor)]

[Expired for OpenStack Object Storage (swift) because there has been no activity for 60 days.]

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/swift/+/803406

[Tim Burke (1-tim-z)]

This is still a problem, and I've at least gotten so far as writing a failing probe test for it: https://review.opendev.org/c/openstack/swift/+/803406

I think we can (ab)use `Expect: 100-continue` and `Transfer-Encoding: chunked` to get a two-phase commit like we've got for PUTs with `If-None-Match: *` -- but it's going to require a bunch of plumbing.