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

[Feat]: for better UX, stream blobs as you fetch when on-demand sync is enabled #1395

Open
rchincha opened this issue Apr 30, 2023 · 6 comments
Assignees
Labels
feature New feature or request rm-external Roadmap item submitted by non-maintainers
Milestone

Comments

@rchincha
Copy link
Contributor

rchincha commented Apr 30, 2023

Is your feature request related to a problem? Please describe.

Currently, we fetch blobs and store locally and then serve. It could cause some clients to error out. So basically, on-demand is mainly a trigger. A better UX for clients would be to stream to them as the blobs are fetched.

Describe the solution you'd like

For this feature, we may have to enhance or replace the containers/image library entirely and use our own custom library.

You may need a pattern like so (or a combo):
https://pkg.go.dev/io#MultiWriter
https://pkg.go.dev/io#TeeReader

Also, a SyncWatcher so that we pull only once and fan-out to pending clients

Describe alternatives you've considered

No response

Additional context

No response

@rchincha rchincha added the feature New feature or request label Apr 30, 2023
@rchincha
Copy link
Contributor Author

@rchincha rchincha added the rm-external Roadmap item submitted by non-maintainers label Apr 30, 2023
@rchincha rchincha added this to the v2.0.0 milestone Apr 30, 2023
@rchincha
Copy link
Contributor Author

rchincha commented May 3, 2023

A blob that is sync'ed is in three possible states

  1. Found, so not need to trigger sync on client pulls
  2. NotFound, so need to trigger sync and add a callback (and store a http.Response ref)
  3. Partial, sync is still in progress and a client is requesting the blob so it must be added to the list maintained in 2.

Keep in mind locking and memory usage (channels, buffers, etc cost us memory)

@rchincha
Copy link
Contributor Author

rchincha commented May 4, 2023

main...rchincha:zot:issue-1395
^ just some quick pseudo-code

@eusebiu-constantin-petu-dbk
Copy link
Collaborator

eusebiu-constantin-petu-dbk commented May 30, 2023

Hello,

I encountered some issues with fsnotify package and with containers/image(used by sync) package while working on this:
so we create a temp directory (uuid.New()) where containers/image will copy images before commiting them to zot's imagestore.

1st issue with fsnotify is that it doesn't support watching dirs recursively, trying to overcome this by adding subdirs to watchlist whenever a CREATE event comes in, will result in races (a CREATE event could be triggered before adding a subdir to watchlist)

2nd issue with containers/image is that they first write to a temp blob file in the form of "oci-put-blob3507476942" before renaming it to the actual blob path.

so I can not identify blobs being written by digest.

basically in sync: we write a blob in a temp file, then move it to a temp directory, then move it to imagestore, trying to keep all these actions synchronized would be really hard.

@elee1766
Copy link
Contributor

@rchincha rchincha modified the milestones: v2.0.1, v2.1.0 Feb 5, 2024
@rchincha
Copy link
Contributor Author

For sync feature, add a "stream: true" to enable this ux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request rm-external Roadmap item submitted by non-maintainers
Projects
None yet
Development

No branches or pull requests

3 participants