Feat/support containerd #1975
Feat/support containerd #1975
Conversation
Signed-off-by: huaiyou <huaiyou.cyz@alibaba-inc.com>
Codecov ReportBase: 17.71% // Head: 17.82% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #1975 +/- ##
==========================================
+ Coverage 17.71% 17.82% +0.11%
==========================================
Files 105 105
Lines 9468 9497 +29
==========================================
+ Hits 1677 1693 +16
- Misses 7563 7573 +10
- Partials 228 231 +3
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. |
| Distributor: distributor, | ||
| ImageEngine: imageEngine, | ||
| Plugins: plugins, | ||
| ContainerRuntimeConfig: cluster.Spec.ContainerRuntime, |
There was a problem hiding this comment.
could we get ContainerRuntime from image spec(image extension) as default value, using cluster.Spec.ContainerRuntime to ovewrite it ?
There was a problem hiding this comment.
i think after we add ContainerRuntime info in image spec, then modify here.
VinceCui
force-pushed
the
feat/support-containerd
branch
from
16ceb5d
to
7063eac
Compare
Signed-off-by: huaiyou <huaiyou.cyz@alibaba-inc.com>
VinceCui
force-pushed
the
feat/support-containerd
branch
from
7063eac
to
31b7843
Compare
…nerd Signed-off-by: huaiyou <huaiyou.cyz@alibaba-inc.com> # Conflicts: # common/common.go # pkg/runtime/kubernetes/join_masters.go
VinceCui
force-pushed
the
feat/support-containerd
branch
3 times, most recently
from
2e58535
to
f007706
Compare
VinceCui
force-pushed
the
feat/support-containerd
branch
2 times, most recently
from
e2ddd5f
to
780bdbe
Compare
| return d.Info, nil | ||
| } | ||
|
|
||
| func (d *DefaultInstaller) getInstallScriptName() string { | ||
| return fmt.Sprintf("%s.sh", d.Type) |
There was a problem hiding this comment.
if container runtime shell file renamed ,is the corresponding rootfs files also need to renamed ?, how to ensure compatibility for the old sealer image.
There was a problem hiding this comment.
Let's make a statement
There was a problem hiding this comment.
the cluster image provider should make sure the script for installing container runtime use the same name with container runtime type.
Signed-off-by: huaiyou <huaiyou.cyz@alibaba-inc.com>
VinceCui
force-pushed
the
feat/support-containerd
branch
from
780bdbe
to
60a5eb4
Compare
| registryURL := net.JoinHostPort(regConfig.Domain, strconv.Itoa(regConfig.Port)) | ||
| if regConfig.Port == 0 { | ||
| registryURL = regConfig.Domain | ||
| } | ||
| cluster.Spec.Env = append(cluster.Spec.Env, fmt.Sprintf("%s=%s", common.EnvRegistryURL, registryURL)) | ||
|
|
||
| if cluster.Spec.ContainerRuntime.Type != "" { | ||
| cluster.Spec.Env = append(cluster.Spec.Env, fmt.Sprintf("%s=%s", common.EnvContainerRuntime, cluster.Spec.ContainerRuntime.Type)) |
There was a problem hiding this comment.
@VinceCui I think read container runtime type by clusterfile is a little complex. if user didn't specify CRI in clusterfile, What will happended? Then if user specify a wrong CRI type in clusterfile which different from clusterimage had, what will happeded?
Here is my suggestion:
now we have LABEL in Kubefile, so we can get CRI info from it. When user use offical image, the CRI info will natural included. If CRI info is not specified in Kubefile, we use docker as a default CRI and give user a hint, if CRI info is wrong, we hint the default CRI info and return error for user.
RFC @starnop
There was a problem hiding this comment.
Thanks for the review.
The first question, if user didn't specify CRI, the default value now is docker, but it may be changed to containerd in future.
The second question, it is no problem to specify the default container runtime by LABEL in the build phase. This does not conflict with the ability to specify the container runtime at runtime. But consider a scenario, what if the image supports multiple container runtimes? ? At this time, it is necessary to allow users to specify at runtime.
感谢review。
第一个问题，如果置为空，默认为docker，但后面可能会改成默认containerd。
第二个问题，在build阶段里LABEL指定默认的容器运行时是没有问题的，这和运行时可以指定容器运行时也是不冲突的，但是考虑一个场景，如果该镜像支持多个容器运行时呢？此时允许使用者在运行时可以指定就是必要的了。
Describe what this PR does / why we need it
Does this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews