Use advisory locks for lockfiles

Lorak-mmk commented 1 year ago

Up until now the locking mechanism used to prevent concurrent downloads was just the existence of a lock file. This implementation had several problems:

Locking procedure was unsafe - it was possible for more than one process to enter critical section at the same time. This was "fixed" by putting random sleep in front of it, but that is not a proper synchronisation mechanism.
Download procedure was clearing whole directory at one point. This caused lockfile to be removed too, so it was possible for another process to enter the critical section at this point.
If the process was killed unexpectedly, the lockfile remained in place. If you try to start the download again it will wait for an hour for the lockfile to be deleted. This is frustrating when running e.g. tests for some driver, as they just hang and you have no idea why.

This PR replaces that with advisory locks and fixes all those problems. Now the download procedure will just restart if it was aborted previously. It's no longer possible for 2 processes to enter critical section, so the (0-5) seconds sleep is removed. Download procedure no longer removes the lockfile.

There is one thing I don't quite understand: why do we use lockfile for Scylla downloads but not for Cassandra downloads? Is there something that makes Cassandra downloads safe in this regard, or is that just an oversight?

fruch commented 1 year ago

Regarding Cassandra, I don't think it's safe either, but we are not using it that much.

I dtest we have used it in 5 tests, and the Cassandra version we use comes pre cached in the docker image we use.

fruch commented 1 year ago

Anyhow I would like to see it running dtest upgrade test that are the cause of introducing this lock, since they are downloading the same versions in multiple test in the same run.

@juliayakovlev can you help @Lorak-mmk getting those run in Jenkins with this ccm ?

Lorak-mmk commented 1 year ago

Addressed review comments - apart from shortening the timeout value as I'm not sure what value would be good here.

fruch commented 1 year ago

Running it on: https://jenkins.scylladb.com/view/staging/job/scylla-staging/job/fruch/job/new-dtest-pytest-parallel/312/

fruch commented 1 year ago

Running it on: https://jenkins.scylladb.com/view/staging/job/scylla-staging/job/fruch/job/new-dtest-pytest-parallel/312/

run pass with success

scylladb / scylla-ccm

Use advisory locks for lockfiles #470