search

containerd / nydus-snapshotter

A containerd snapshotter with data deduplication and lazy loading in P2P fashion
https://nydus.dev/
Apache License 2.0
173 stars 100 forks source link

A more easy way to setup nydus snapshotter by DaemonSet #570

Closed ChengyuZhu6 closed 10 months ago

ChengyuZhu6 commented 11 months ago

Confidential Containers leverage nydus snapshotter to pull images within the guest using fs_driver=proxy and share images on the host using fs_driver=blockdev to ensure data confidentiality. However, when running related tests for Kata CI with nydus snapshotter, we faced challenges installing and uninstalling nydus snapshotter on various architectures and configuring the containerd settings. We envisioned a smoother experience, where nydus snapshotter provides a daemonset, enabling users to seamlessly run and clean up nydus snapshotter and making it more easy.

Fixes #565

ChengyuZhu6 commented 11 months ago

/cc @jiangliu @sctb512 @imeoer @adamqqqplay

adamqqqplay commented 11 months ago

@ChengyuZhu6 Thanks for your great work! It will take some time to review.

imeoer commented 11 months ago

It seems to break some tests, let's take a look first.

imeoer commented 11 months ago

Cloud we enhance the helm chart? We can provide the options to configure the snapshotter fs_driver.

ChengyuZhu6 commented 11 months ago

Cloud we enhance the helm chart? We can provide the options to configure the snapshotter fs_driver.

Yes, I plan to use helm for installing snapshotter, but it requires a different repo. So I have proposed an alternative method that does not rely on helm. Next, I will submit a PR to the helm repo.

imeoer commented 11 months ago

It seems the CI still needs to be fixed: https://github.com/containerd/nydus-snapshotter/actions/runs/7269460179/job/19807116165?pr=570 :)

wainersm commented 10 months ago

@ChengyuZhu6 I've finished my review and left a couple of comments. I didn't find any bug but I'm afraid I won't have time to give it a try. Thanks for working on this && amazing work!

fidencio commented 10 months ago

@ChengyuZhu6, one thing that I'm missing from your patches is the ability to start the nydus as a systemd service.

While this may not be used by everyone, we should give the users the ability to do as this is way more reliable than just directly calling a binary.

I'd suggest to add an option for that, as that's most likely what we'll use on the Confidential Containers side.

ChengyuZhu6 commented 10 months ago

@ChengyuZhu6, one thing that I'm missing from your patches is the ability to start the nydus as a systemd service.

While this may not be used by everyone, we should give the users the ability to do as this is way more reliable than just directly calling a binary.

I'd suggest to add an option for that, as that's most likely what we'll use on the Confidential Containers side.

Sure. I'll add it.

ChengyuZhu6 commented 10 months ago

@ChengyuZhu6, one thing that I'm missing from your patches is the ability to start the nydus as a systemd service.

While this may not be used by everyone, we should give the users the ability to do as this is way more reliable than just directly calling a binary.

I'd suggest to add an option for that, as that's most likely what we'll use on the Confidential Containers side.

Done.

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0397a1f) 33.63% compared to head (e0ba512) 33.52%. Report is 8 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/containerd/nydus-snapshotter/pull/570/graphs/tree.svg?width=650&height=150&src=pr&token=R3RX5WX6R1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=containerd)](https://app.codecov.io/gh/containerd/nydus-snapshotter/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=containerd) ```diff @@ Coverage Diff @@ ## main #570 +/- ## ========================================== - Coverage 33.63% 33.52% -0.12% ========================================== Files 65 65 Lines 8259 8287 +28 ========================================== Hits 2778 2778 - Misses 5166 5194 +28 Partials 315 315 ``` [see 1 file with indirect coverage changes](https://app.codecov.io/gh/containerd/nydus-snapshotter/pull/570/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=containerd)
imeoer commented 10 months ago

Thanks for the work!

ChengyuZhu6 commented 10 months ago

In k8s auth e2e tests, we run snapshotter as systemd service in cri auth test and run snapshotter as standalone process in kube auth test to ensure all two mode can run as expected.

ChengyuZhu6 commented 10 months ago

@imeoer I have made the changes you requested. Please take another look and let me know if there is anything else I need to do. I appreciate your feedback and guidance.

ChengyuZhu6 commented 10 months ago

@fidencio @wainersm @zvonkok @liudalibj I made some changes after your reviews that I decided to avoid using toml-cli to edit toml files, since it only works on the x86 architecture.

imeoer commented 10 months ago

We'd better to standardize all the binary paths to /usr/local/bin/* and all the configuration paths to /etc/nydus/*.json, others LGTM, thanks! Let's provide a helm chart to further reduce the difficulty of the deploy steps.

imeoer commented 10 months ago

Thanks @ChengyuZhu6 for the work and all reviewers @fidencio @wainersm @zvonkok @beraldoleal !