-
Notifications
You must be signed in to change notification settings - Fork 116
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]filesystem benchmark #101
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for this.
Can we measure it towards dockerhub? However, the main concern is we will end up to make many HTTP requests to the registry...
And maybe we can include comparison with other filesystems.
cc: @AkihiroSuda
Yes, dockerhub is a much more general case, I'll make it to dockerhub. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. We need further investigation for finding the bottleneck but definitely we must improve the performance. I'll take a deeper look at it this week.
script/fs-bench/work/run.sh
Outdated
IMAGE_LEGACY="${IMAGE_LEGACY:-docker.io/sequix/fio:legacy_256m_4t}" | ||
IMAGE_STARGZ="${IMAGE_STARGZ:-docker.io/sequix/fio:stargz_256m_4t}" | ||
IMAGE_ESTARGZ="${IMAGE_ESTARGZ:-docker.io/sequix/fio:estargz_256m_4t}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use docker.io/stargz
organization here. I'll push these images to docker.io/stargz
later.
rebaseed and signed off. |
@sequix After a deeper investigation last week, it turned out that the bad read performance (#101 (comment)) on the filesystem didn't come from your benchmark method but did come from some bugs in the filesystem. Thanks a lot for your testing! And can you add Apache 2.0 license headers for the following files? They are needed to pass CI tests. Please refer to other existing files.
I uploaded benchmarking images on https://hub.docker.com/r/stargz/fio |
How can I check the golint error log? GitHub action did not provide much info to help me pass the CI. |
cba73c9
to
b775cd7
Compare
Golint output is supposed to be logged to Github Actions. But in terms of header checks, we are currently logging just a list of files that haven't valid headers so we might need more verbose or friendly logging for this (but currently the list is enough as long as we know it indicates "these files have no valid headers"). |
Seems the |
Recent test flaky seems to be because of recent updates of one of the images ( |
rebased |
To make a contrast, this benchmark will test all three types of image, OCI, stargz and estargz in following steps. 1.Use fio to generate fake pread() requests parallely (4 threads). 2.Scrape metrics in /proc/<pid of stargz-snapshotter>. 3.Calculate scraped metrics with PromQL. 4.Draw fio bandwidth-latency and process-metrics images with gunplot. Signed-off-by: Chuang Zhang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix on the scraping codes. Smoke tested and found some points to fix.
I'll take a deeper look at the codes related to processing data this week. I'm also not sure that do we need rate()
function for our use-case. process/main.go
will be much simpler if we don't use PromQL.
RUN git clone https://github.com/opencontainers/runc \ | ||
$GOPATH/src/github.com/opencontainers/runc && \ | ||
cd $GOPATH/src/github.com/opencontainers/runc && \ | ||
git checkout d736ef14f0288d6993a1845745d6756cfc9ddd5a && \ | ||
GO111MODULE=off make -j2 BUILDTAGS='seccomp apparmor' && \ | ||
GO111MODULE=off make install && \ | ||
git clone https://github.com/containerd/containerd \ | ||
$GOPATH/src/github.com/containerd/containerd && \ | ||
cd $GOPATH/src/github.com/containerd/containerd && \ | ||
git checkout 990076b731ec9446437972b41176a6b0f3b7bcbf && \ | ||
GO111MODULE=off make -j2 && GO111MODULE=off make install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recently we introduced a common base image among test codes for easier version management of containerd & runc in this project. Can we use the base image for this? Can use snapshotter-base
build target which includes runc + containerd + containerd-stargz-grpc(built from the repo) so we won't need to build the snapshotter binary inside the testing container during runtime.
Please also see the script in the integration test.
printf "\n" | ||
else | ||
printf "unset key\n" | ||
printf 'plot "%s" w l lw 2\n' "$LOGS_BW" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$LOGS_BW
=> $LOGS
?
# Set environemnt variable if you want use a customize fio image, | ||
# whose entrypoint must be the start of a fio test, and output all its logs | ||
# (stdio (in file `stdio`), bw_log, iops_log, and lat_log) to /output. | ||
# 256m_4t stands for 4 threads and each read 256MiB (1024MiB in total). | ||
IMAGE_LEGACY="${IMAGE_LEGACY:-docker.io/stargz/fio:legacy_256m_4t}" | ||
IMAGE_STARGZ="${IMAGE_STARGZ:-docker.io/stargz/fio:stargz_256m_4t}" | ||
IMAGE_ESTARGZ="${IMAGE_ESTARGZ:-docker.io/stargz/fio:estargz_256m_4t}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix the naming convention of fio images to be fio:256m-4t-{org,sgz,esgz}
(stands for 4 threads and each read 256MiB)?
Please see also: https://hub.docker.com/r/stargz/fio/tags
return | ||
} | ||
|
||
ts := " " + strconv.FormatInt(now.Unix()*1000+int64(now.Nanosecond())/1e6, 10) + " " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we do now.UnixNano() / int64(time.Millisecond)
?
kill_all "containerd" | ||
kill_all "containerd-stargz-grpc" | ||
kill_all "scrape" | ||
if [ "$NOCLEANUP" == "-nocleanup" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unbound variable when no option is passed. It should be ${NOCLEANUP:-}
.
else | ||
cleanup | ||
fi | ||
if [ "${NOSNAPSHOTTER}" == "-nosnapshotter" ] ; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the above. Should be ${NOSNAPSHOTTER:-}
.
I am trying to write a filesystem test suite for this project. Basically, it uses fio to generate fake I/O operations, then stargz-snapshotter range requests a local registry through eth0 with a limited bandwidth. Meanwhile, metrics in /proc will scraped and processed afterward using prometheus and gnuplot, to generate image about the stargz-snapshotter process like:
fio will record bandwidth, iops and latency also. These will be painted with gnuplot too:
The two pictures above is made from a fio test within a stargz image, which started 4 threads to read a same file until up to 512MiB.