search

krobertson / deb-s3

Easily create and manage an APT repository on S3 -- NO LONGER MAINTAINED
MIT License
482 stars 148 forks source link

Locking fails when uploading without an architecture type #97

Open jjshoe opened 8 years ago

jjshoe commented 8 years ago

Steps to reproduce:

  1. Add a sleep(60) here: https://github.com/krobertson/deb-s3/blob/master/lib/deb/s3/cli.rb#L233
  2. Run deb-s3 upload -l -p -b your-bucket -v private your-package.deb
  3. While it's sleeping in your-bucket/dists/stable/main you'll discover a binary- folder holding the lockfile

The fix should be as simple as getting a list of all arch types up front, but this is complicated. Reasons:

  1. A user can specify an arch type, this over rides the arch type on the package. https://github.com/krobertson/deb-s3/issues/95
  2. Manifests with no packages to upload, are still being uploaded. https://github.com/krobertson/deb-s3/issues/96
jjshoe commented 8 years ago

@alexmreis

jjshoe commented 8 years ago

And by simple, I mean, getting a list of all arch types, getting a lock on each, storing that an array, and looping to unlock.

alexmreis commented 8 years ago

I'd suggest the fix would actually be requiring the architecture to be explicitly specified if -l is used and exiting with an error if it isn't. Having multiple locks increases the complexity of something that is already fragile. That said if you consistently don't specify the architecture, then the behaviour of the locking mechanism is still consistent, albeit a bit dirty to the repo layout.

The eventual consistency for removal of the lock doesn't matter much, as it would only be potentially dangerous to actually go ahead and write things to the repository while someone else is also doing it. There is a built-in polling wait mechanism in -l that retries to acquire a lock for some time before giving up.

The -l option always checks for an existing lock first, and if and only if a lock is not found does it try to write a new lockfile, so a replace wouldn't ever occur. Read-after-write consistency is good enough for the purposes of at least adding packages. I strongly advise against deleting any without revoking permissions to the bucket to all other users first.

Could you please elaborate on the issue you have uncovered regarding the eventual consistency @jjshoe ?

jjshoe commented 8 years ago

@alexmreis I deleted the eventual consistency comment from here, I'll make a new issue when I come up with a great way to reproduce.

jjshoe commented 8 years ago

@alexmreis also, see https://github.com/krobertson/deb-s3/issues/95.

jjshoe commented 8 years ago

@alexmreis - Actually, I have the nail in the coffin, and the safest possible way for you to do this anyway. You should lock at the bucket level. I know, then it's not as fast as it could be, but bucket-name/dists/stable/Release gets updated every run as well.

jjshoe commented 8 years ago

Also worth noting, specifying an architecture type out front makes it difficult to upload a whole mess of deb files of different arch types.

alexmreis commented 8 years ago

A bucket level lock breaks the idea of multiple repositories in the same bucket that the --prefix option allows for. It should at least be on the prefix level, but I do like the idea @jjshoe

jjshoe commented 8 years ago

@alexmreis yes, sorry, I'm all for prefix, replace bucket with repository.