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

New design for basefs. #1909

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

VinceCui
Copy link
Collaborator

@VinceCui VinceCui commented Dec 7, 2022

Signed-off-by: huaiyou huaiyou.cyz@alibaba-inc.com

Describe what this PR does / why we need it

In order to facilitate the use of basic image creators, we need to standardize the definition of sealer's rootfs.

Does this pull request fix one issue?

Describe how you did it

I mainly made the following designs:

  1. The purposes of bin, etc, and plugins are basically the same, but the usage method is clarified.
  2. Clarify the purpose of Metadata, README.md, and imageList.
  3. Two new directories, yamls and charts, have been added to provide users with the ability to expand the basic image more flexibly.
  4. Finally, the purpose of different scripts under scripts is clarified.

Describe how to verify it

Special notes for reviews

为了方便基础镜像制作者的使用,我们需要标准化sealer的rootfs的定义。

我主要做了以下设计:

  1. bin,etc,plugins这几个目录的用途基本不变,只是明确了使用方式。
  2. 明确scripts下的不同脚本的用途。

Signed-off-by: huaiyou <huaiyou.cyz@alibaba-inc.com>
@codecov-commenter
Copy link

Codecov Report

Base: 21.11% // Head: 21.70% // Increases project coverage by +0.58% 🎉

Coverage data is based on head (cf7b411) compared to base (fbd817f).
Patch coverage: 39.47% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1909      +/-   ##
==========================================
+ Coverage   21.11%   21.70%   +0.58%     
==========================================
  Files          74       78       +4     
  Lines        6762     7055     +293     
==========================================
+ Hits         1428     1531     +103     
- Misses       5148     5322     +174     
- Partials      186      202      +16     
Impacted Files Coverage Δ
build/kubefile/parser/kubefile.go 53.44% <0.00%> (-1.91%) ⬇️
pkg/cluster-runtime/hook.go 3.75% <0.00%> (-0.05%) ⬇️
pkg/image/reference/util.go 83.78% <ø> (ø)
pkg/logger/formatter.go 81.13% <ø> (ø)
pkg/clusterfile/clusterfile.go 28.45% <14.89%> (-5.99%) ⬇️
build/kubefile/parser/utils.go 60.75% <53.68%> (-10.67%) ⬇️
build/kubefile/parser/app_handler.go 47.36% <100.00%> (-2.03%) ⬇️
pkg/logger/logger.go 40.00% <0.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: huaiyou <huaiyou.cyz@alibaba-inc.com>
Signed-off-by: huaiyou <huaiyou.cyz@alibaba-inc.com>
│ ├── runc
│ └── vpnkit
├── etc
├── etc # configs will be put into all nodes' sealer-rootfs
Copy link
Member

Choose a reason for hiding this comment

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

What does sealer-rootfs? Could you help to define that? Since there are quite a lot of rootfs in sealer's concept. @VinceCui

@@ -4,7 +4,7 @@ cloud rootfs will package all the dependencies refers to the kubernetes cluster

```shell script
.
├── bin
├── bin # binaries will be installed at all nodes' /usr/local/bin
Copy link
Member

Choose a reason for hiding this comment

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

If we only store binaries in this directory, I am wondering whether it is proper to container scripts here, like containerd-rootless-setuptool.sh, containerd-rootless.sh and kubelet-pre-start.sh.

In addition, binary may not be compatible with plarforms(x86, arm). Then in the design, we make a ClusterImage to be compatible to all arch platforms or different ClusterImage compatible to different platforms.

It could be better that we have a top-down through design, I think. 🐱

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with that. It's weird to have a bunch of shell scripts in a directory called bin

├── plugins # plugins can run on some hooks, such as pre-init-host, post-install, see more in the plugins documentation
│ └── disk_init_shell_plugin.yaml
├── scripts # scripts can use all ClusterFile's env as Linux env variables
│ ├── init-kube.sh # initialize kube* binaries on target hosts
Copy link
Collaborator

@starnop starnop Jan 5, 2023

Choose a reason for hiding this comment

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

Could you please add more information to the scripts, including input, output, and functions should be included?

It can be described in a separate section.

@@ -13,44 +13,28 @@ cloud rootfs will package all the dependencies refers to the kubernetes cluster
│ ├── kubectl
│ ├── kubelet
│ ├── nerdctl
│ ├── kubelet-pre-start.sh
│ ├── helm
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if a dual-architecture cluster image, such as support for both amd64 and arm64?

I think we should have a relatively complete example.

@@ -4,7 +4,7 @@ cloud rootfs will package all the dependencies refers to the kubernetes cluster

```shell script
.
├── bin
├── bin # binaries will be installed at all nodes' /usr/local/bin
Copy link
Collaborator

Choose a reason for hiding this comment

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

In my opinion, the binaries should be installed at the system PATH of all nodes, and the specific path should be compatible with the operating system which is determined by the installation script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants