ostree/prune: Calculate reachability under exclusive lock

[jlebon]

When we calculate the reachability set in ostree prune, we do this
without any locking. This means that between the time we build the set
and when we call ostree_repo_prune_from_reachable, new content
might've been added. This then causes us to immediately prune that
content since it's not in the now outdated set.

Fix this by calculating the set under an exclusive lock.

I think this is what happened in
fedora-silverblue/issue-tracker#405. While
the pruner was running, the new-updates-sync script[1] was importing
content into the repo. The newly imported commits were immediately
deleted by the many ostree prune --commit-only calls the pruner does,
breaking the refs.

[1] https://pagure.io/fedora-infra/ansible/blob/35b35127e444/f/roles/bodhi2/backend/files/new-updates-sync#_18

[cgwalters]

Ah, yes. Classic GC issue. The obvious optimization here would be to skip pruning any commits created (filesystem timestamp I guess) after the scan started.

But for now, SGTM.

[jlebon]

Ah, yes. Classic GC issue. The obvious optimization here would be to skip pruning any commits created (filesystem timestamp I guess) after the scan started.

But for now, SGTM.

Good idea. Let me try this and see how hard it'd be. It's definitely unfortunate to be locking for the whole operation since it could take a long time to calculate reachability.

Hmm, though I guess the timestamp trick won't work if you're somehow using rsync for getting content into the repo and preserving timestamps (I think this has come up before IIRC). I wonder if what we actually want here is a new OstreeRepoPruneOptions field where the client also passes the list of objects to consider for pruning. So we'd list the objects, then calculate reachability, and then pass the objects and the set into the prune API.

[jlebon]

I wonder if what we actually want here is a new OstreeRepoPruneOptions field where the client also passes the list of objects to consider for pruning. So we'd list the objects, then calculate reachability, and then pass the objects and the set into the prune API.

OK, updated this to do that now! It seems safer than the timestamp trick but does require more invasive public API changes.

[jlebon]

Well this is awkward... make gir doesn't generate code that respects rustfmt. Fixed!

[jlebon]

OK I ended up going back to just using an exclusive lock for this. The issue with the previous approach (listing objects and passing it in) is that it doesn't really solve the race issue because refs are usually the last thing that get updated when content is imported into the repo, so there's still a time where objects will appear unreferenced.

The timestamp trick could be made to work, but (1) it would only work on archive repos where the object files don't have canonicalized timestamps (in the non-archive case, we could scope it to just handling the --commit-only case), and (2) it seems risky to rely too much on filesystem timestamps (e.g. NTP adjustments, or some promotion workflow where timestamps are copied from elsewhere such as if using rsync -a). We could have it work in a follow-up, but I think it needs to be opt-in.

Also, benchmarking how much time the reachability calculation step takes revealed that it's far less expensive than the actual ostree_repo_prune_from_reachable call (where we already take an exclusive lock anyway), so it's not as valuable as initially thought to try to optimize this.

[cgwalters]

/retest