Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - detect copy operations by other processes #619

Closed
wants to merge 1 commit into from

Conversation

vrothberg
Copy link
Member

containers/image#607 and containers/storage#307 must be merged before this PR.

Requires to vendor containers/image, storage and the latest
version of the progress bar library.

Signed-off-by: Valentin Rothberg [email protected]

@vrothberg
Copy link
Member Author

vrothberg commented Mar 20, 2019

Here's an example how to test it:
asciicast

@edsantiago
Copy link
Member

I'm obviously doing something wrong; I can't get my skopeo to recognize a simultaneous pull:

$ git show
commit dd70b2a9f846f45f4381d45d4df36e7d77d4db9c (HEAD -> pr/619)
...
$ make binary-local
go build "-buildmode=pie" -ldflags "-X main.gitCommit=dd70b2a9f846f45f4381d45d4df36e7d77d4db9c" -gcflags "" -tags "   ostree " -o skopeo ./cmd/skopeo
$ sudo ./skopeo copy docker://fedora:29 containers-storage:fedora:esm1   [and, in other window, esm2]

In all my attempts, both skopeos pull the entirety of 01eb078129a0. Do you see any obvious problems with my setup?

@vrothberg
Copy link
Member Author

@edsantiago my apologies! As @mtrmac pointed out here containers/image#607 (comment), it's the code not you. I simply missed committing the last s/false/true/ to the PR at containers/image. Sorry! I shouldn't commit at the end of the day 🙉

@edsantiago
Copy link
Member

That works better, thank you!

The word 'stopped' is pretty strong; would you consider 'paused' instead? And perhaps 'being copied' to indicate ongoing activity?

@vrothberg
Copy link
Member Author

That works better, thank you!

The word 'stopped' is pretty strong; would you consider 'paused' instead? And perhaps 'being copied' to indicate ongoing activity?

Sounds great, thanks!

@vrothberg
Copy link
Member Author

@rhatdan @mtrmac @nalind @edsantiago, please have another look:

Besides several cleanups of the code, I performed the following changes:

  • Storage does not expose digest-specific Lock() - Unlock() API anymore but has a GetDigestLock(path string) (Locker, error) API that we use in storageImageDestination.
  • File descriptors do not leak anymore as the lock files are now opened and closed on demand. We're already reference counting in the lockfile implementation. When Unlocking the last remaining reference on a lock we're closing the file descriptor.
  • The digest locks are being tracked in storageImageDestination and all locked locks (but not yet unlocked ones) are unlocked in storageImageDestination.Close() which is being invoked in a deferred call in copy.Image() and effectively avoids deadlocks in the precense of errors or panics.

Requires to vendor containers/image, storage and the latest
version of the progress bar library.

Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

Closing in favour of containers/image#611.

@vrothberg vrothberg closed this Apr 18, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants