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

Refactor the optimizer based on containerd #240

Merged
merged 1 commit into from
Jan 30, 2021

Conversation

ktock
Copy link
Member

@ktock ktock commented Jan 13, 2021

#223
#230
#238

This refactors optimizer based on continerd and fanotify. Options of ctr-remote image optimize are unchanged except it now accepts --oci option and multiple --platform options. Now ctr-remote image optimize requires containerd daemon up-and-running but doesn't require FUSE. CAP_SYS_ADMIN is needed for fanotify and cloning a mount namespace.

The design is the following:

  1. *analyzer.Analize() creates a mount namespace and monitor "/" mountpoint of that namespace with fanotify-based notifier process (ctr-remote notify).
  2. Then *analyzer.Analyze preapres snapshots and container spec based on the specified option. Then it runs a container in that mount namespace created in step 1 (notifier process is invisible to the container). Notifier sends all accessed files to the optimizer over stdio.
  3. nativeconverter converts layers based on these accesssed files (= prioritized files).

NOTEs:

  • The reason we pre-create mount namespace and let the container use it is that fanotify doesn't notify file accesses across namespaces.
  • Notifier monitors a single flat rootfs so it notifies accessed files to optimizer without layer index. Here, *analyzer.imageRecorder takes responsibility to find out the layer that the file notified belongs to.
    • Initially I thought if we can do fanotify for each lower layer of the overlay mount but fanotify doesn't seem to notify anything even accesses occur on the merged directory.

TODOs:

  • Update tests
  • Refactor codes
    • Step 2 can be refactored using containerd lib's options (containerd.WithXXXX) instead of analyzer.runtimeOpts.
  • --wait-on-signal doesn't seem to work yet.
  • add & update docs
  • Measure the benchmark with eStargz optimized by this optimizer
  • Add "Optimizer In Container" (following-up)
  • Come up with better communication protocol between optimizer and notifier (following-up?).
  • Enable nativeconverter (estargz) to specify per-layer option indexed by layer index (not by layer digest) because it's possible that the same two layers in a single image have same digests (following-up?).

cc @AkihiroSuda

)

var NotifyCommand = cli.Command{
Name: "notify",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/notify/fanotify/ might have more clarity

@AkihiroSuda
Copy link
Member

Thanks, design SGTM 👍

@@ -23,6 +23,8 @@ ORG_IMAGE_TAG="${REGISTRY_HOST}:5000/test:org$(date '+%M%S')"
OPT_IMAGE_TAG="${REGISTRY_HOST}:5000/test:opt$(date '+%M%S')"
NOOPT_IMAGE_TAG="${REGISTRY_HOST}:5000/test:noopt$(date '+%M%S')"
TOC_JSON_DIGEST_ANNOTATION="containerd.io/snapshot/stargz/toc.digest"
NETWORK_MOUNT_TEST_ORG_IMAGE_TAG="ghcr.io/stargz-containers/centos:8-test"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: CentOS is deprecated

@ktock ktock force-pushed the optimize-ctd branch 2 times, most recently from 14ae5bb to 9c2288a Compare January 18, 2021 16:02
@ktock
Copy link
Member Author

ktock commented Jan 18, 2021

@mc256 Sorry for the slow reply. I implemented --wait-on-signal for non-terminal mode. PTAL.

@ktock ktock force-pushed the optimize-ctd branch 4 times, most recently from 6881173 to 770ff66 Compare January 21, 2021 10:44
@ktock ktock force-pushed the optimize-ctd branch 2 times, most recently from 9f86fdb to a55cf0c Compare January 29, 2021 08:38
@ktock
Copy link
Member Author

ktock commented Jan 29, 2021

Benchmarking result with images generated by this new optimizer seems fine.

https://github.com/ktock/stargz-snapshotter/runs/1789973644

result

@ktock ktock marked this pull request as ready for review January 29, 2021 08:41
@AkihiroSuda
Copy link
Member

Merging, but test script should show error on non-amd64

@AkihiroSuda AkihiroSuda merged commit 0d28df3 into containerd:master Jan 30, 2021
@ktock ktock deleted the optimize-ctd branch October 7, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants