sync controller: replace revision file with full diff each interval

[skriss]

Signed-off-by: Steve Kriss [email protected]

Closes #1343

This is kind of what I was thinking ref. #1343 (comment). Instead of relying on the revision file to tell us when BSL contents change, we get the full list of backups from the BSL (this is just a single API call to the object store), and do a setwise comparison (based on name) to the backups in the cluster. We then sync/import any ones that are in the BSL but not in the cluster.

I also removed the limitation that the sync period must be >= 1 minute - I figure users should be able to set it lower if they want.

I haven't yet removed the actual updating of the revision file when backups happen - we could choose to leave that in for now, or remove it entirely.

If this makes sense, I'll proceed with tests/etc.

[prydonius]

+1 to this approach, I think this is much much simpler!

[skriss]

I fixed the tests, and removed the code that gets/sets the metadata/revision file in object storage.

The only other thing to consider doing is adding some code to remove any existing metadata/revision files, or we could just add a script/instructions to the release notes to do this - any thoughts? It's not hurting anything by being there, will just be ignored.

[prydonius]

The only other thing to consider doing is adding some code to remove any existing metadata/revision files, or we could just add a script/instructions to the release notes to do this - any thoughts? It's not hurting anything by being there, will just be ignored.

Seems like we could just leave the older metadata/revision files there, unless we think they might be used by some other tooling and it doesn't make sense to keep out-of-date revision files around (in which case this would be breaking anyway).

[carlisia]

Much simpler. 👍

[carlisia]

I'm fine with leaving the revision file in backups from past versions.

[nrb]

I like the simpler approach!

Have you observed any time differences (based on log messages would be simplest, I suppose) between the old approach and this one?

[skriss]

Have you observed any time differences (based on log messages would be simplest, I suppose) between the old approach and this one?

Haven't specifically looked, though performance seemed fine/~instantaneous in the testing I did. I can try with some bigger data sets.

[skriss]

Tweaked logging so it only info-logs if there are actually backups to sync into the cluster for a location; otherwise, debug-logs.

[skriss]

Have you observed any time differences (based on log messages would be simplest, I suppose) between the old approach and this one?

The algorithm I'm using now should actually be faster than the old one -- previously, when syncing a location, we'd first get a list of all backups in the bucket, then make a K8s API call per backup in the BSL to check if that backup exists in-cluster. Now, I'm making a single List call (to a lister) upfront to get all backups in the cluster, and then doing an in-memory set diff to figure out which ones should be synced in. So a bunch of over-the-wire API calls are saved.

[carlisia]

@nrb PTAL.