storage: GetBlob: write to a local tempfile#1167
Conversation
|
LGTM |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
(Just a drive-by, I don’t have an opinion on the performance trade-off.)
vrothberg
force-pushed
the
getblob-optimization
branch
from
90cbe6f to
1da9d8f
Compare
|
@vrothberg could you perhaps suggest a test to measure this change? And would it make sense to include such a test in CI ? /cc @ashcrow |
It's rather tough to test such performance-oriented changes. A simple manual test is to force a long copy operation to a registry (e.g., podman push) and in parallel accessing the storage (e.g., podman images). The (expected) result is that second operation is blocked only for a short period of time in contrast to being blocked for the entire copy/push operation as before. |
Let's wait a bit :^) |
vrothberg
force-pushed
the
getblob-optimization
branch
2 times, most recently
from
1da9d8f to
61d68ad
Compare
|
LGTM |
lsm5
left a comment
lsm5
left a comment
There was a problem hiding this comment.
Change LGTM. I'll try a test with this change and get back if I see anything. Guess we gotta wait anyway for the rate-limiting issue.
When reading blobs (e.g., layers) from the storage, write the data to a temporary file first and return its file descriptor. The main goal is to keep the time of the storage being locked as short as possible. With this approach we are disk-bound which is generally faster than the being network bound. The motivation for this change is the desire to allow for accessing the storage during pushes. We have attempted to optimize certain execution paths in containers/storage but those require the callers to know whether data will be manipulated or read, which is far from being trivial. The daemon-less nature of the storage also requires to write certain data. This approach here is entirely transparent to the storage. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
vrothberg
force-pushed
the
getblob-optimization
branch
from
61d68ad to
20f7287
Compare
|
Green, and happy. |
lsm5
left a comment
lsm5
left a comment
There was a problem hiding this comment.
Tested locally just now, this is really nice. podman images and other commands don't have to wait for podman push anymore!
LGTM.
|
I want to hit merge, but just checking if it's ok to |
"Rebase + merge" is the default in c/image. Thanks for reviewing and testing! |
|
How about using a hard link to avoid unnecessary disk writes? |
we'd need to make sure the same file system is used. For that, we could use the new |
The data is typically a stream (formed at runtime with tar-split data), not a file that could be linked. |
7 participants
When reading blobs (e.g., layers) from the storage, write the data to a
temporary file first and return its file descriptor.
The main goal is to keep the time of the storage being locked as short
as possible. With this approach we are disk-bound which is generally
faster than being network bound.
The motivation for this change is the desire to allow for accessing the
storage during pushes. We have attempted to optimize certain execution
paths in containers/storage but those require the callers to know
whether data will be manipulated or read, which is far from being
trivial. The daemon-less nature of the storage also requires to write
certain data.
This approach here is entirely transparent to the storage.
Signed-off-by: Valentin Rothberg rothberg@redhat.com