Feature: k0s runtime remain work #1686

starComingup · 2022-09-05T03:06:46Z

Describe what this PR does / why we need it

After k0s init step, there are update/join/delete/reset interface which need to implement, this PR finish all k0s cluster install action.

Does this pull request fix one issue?

NONE, but a step for #1399

justadogistaken · 2022-09-05T07:07:43Z

pkg/runtime/k0s/delete_masters.go

+	RemoveK0sBin              = "rm -rf /usr/bin/k0s"
+	RemoteRemoveEtcHost       = "sed -i \"/%s/d\" /etc/hosts"
+	RemoteRemoveRegistryCerts = "rm -rf " + DockerCertDir + "/%s*"
+	KubeDeleteNode            = "kubectl delete node %s"


I'd rather put those command at the caller place. This will break the readability.

Got it! next commit will fix it!

justadogistaken · 2022-09-05T07:08:55Z

pkg/runtime/k0s/delete_masters.go

+	for _, master := range masters {
+		master := master
+		eg.Go(func() error {
+			master := master


I guess there is no need to declare the temp var again.

codecov-commenter · 2022-09-05T07:48:20Z

Codecov Report

Base: 18.41% // Head: 18.41% // No change to project coverage 👍

Coverage data is based on head (a487276) compared to base (7f76157).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1686   +/-   ##
=======================================
  Coverage   18.41%   18.41%           
=======================================
  Files          66       66           
  Lines        5354     5354           
=======================================
  Hits          986      986           
  Misses       4242     4242           
  Partials      126      126

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.

allencloud · 2022-09-06T06:15:27Z

pkg/runtime/k0s/delete_masters.go

+	RemoveKubeConfig          = "rm -rf /usr/bin/kube* && rm -rf ~/.kube/"
+	RemoveK0sBin              = "rm -rf /usr/bin/k0s"
+	RemoteRemoveEtcHost       = "sed -i \"/%s/d\" /etc/hosts"
+	RemoteRemoveRegistryCerts = "rm -rf " + DockerCertDir + "/%s*"


Please do not use format in the const string value. It would make code quite complicated.

allencloud · 2022-09-06T06:16:10Z

pkg/runtime/k0s/delete_masters.go

+	RemoveK0sBin              = "rm -rf /usr/bin/k0s"
+	RemoteRemoveEtcHost       = "sed -i \"/%s/d\" /etc/hosts"
+	RemoteRemoveRegistryCerts = "rm -rf " + DockerCertDir + "/%s*"
+	KubeDeleteNode            = "kubectl delete node %s"


It should use an API calling via k8s apiclient torwards apiserver, rather than using a shell command.

allencloud · 2022-09-06T06:18:31Z

pkg/runtime/k0s/delete_masters.go

+			master := master
+			logrus.Infof("Start to delete master %s", master)
+			if err := k.deleteMaster(master); err != nil {
+				logrus.Errorf("failed to delete master %s: %v", master, err)


You should return err here, otherwise the eg.Go() could not catch the detailed error, and the return eg.Wait() would always return nil, which is definitely unreasonable.

allencloud · 2022-09-06T06:26:32Z

pkg/runtime/k0s/delete_masters.go

+func (k *Runtime) deleteMaster(master net.IP) error {
+	ssh, err := k.getHostSSHClient(master)
+	if err != nil {
+		return fmt.Errorf("failed to delete master: %v", err)


I think your error log message should be optimized. When logging error message, you should set up a rule for yourself, where to locate the wrap message. In this case, the wrap message is failed to delete master:. And in Line 49, you have code logrus.Errorf("failed to delete master %s: %v", master, err). Then if an error is returned back, then you error message would be failed to delete master 1.1.1.1: failed to delete master: xxxxxx. It is duplicated and definitely inproper.

I wish that you could pay more attention on every part of the code. Only by patience and carefulness, could quality of software be achieved.

allencloud · 2022-09-06T06:30:41Z

pkg/runtime/k0s/delete_masters.go

+	return eg.Wait()
+}
+
+func (k *Runtime) deleteMaster(master net.IP) error {


This is a detailed deep implementation of delete a master node. I think we should explain clearly what is the STEPS to delete a master by comments. If we understand the steps and then the code review could be quite easy. Actually we should review the design firstly, and code secondly. No design there, and we can hardly do the code review. Do you think so ?

Yep! when I read kubernetes runtime module, I have the same feel. which files to delete? and what logic to follow. I so sorry to made this, fix this ASAP.

allencloud · 2022-09-06T06:33:07Z

pkg/runtime/k0s/delete_masters.go

+}
+
+func (k *Runtime) isHostName(master, host net.IP) (string, error) {
+	hostString, err := k.CmdToString(master, "kubectl get nodes | grep -v NAME  | awk '{print $1}'", ",")


Please use k8s apiClient to get the information you want. Otherwise it is quite strange to add shell action in golang code. If KubeCtl released a higher version, and the output format changes, the code here will immediately meet the compatibility issue. What a bad design. Right?

allencloud · 2022-09-06T06:34:44Z

pkg/runtime/k0s/delete_masters.go

+	for _, h := range hosts {
+		if strings.TrimSpace(h) == "" {
+			continue
+		} else {


Since there is a continue in if block, you can remove the else to remove indent code.

if strings.TrimSpace(h) == "" { continue } hh := strings.ToLower(h) fromH := strings.ToLower(hostName) if hh == fromH { name = h break }

allencloud · 2022-09-06T06:38:20Z

I just reviewed the delete_master.go file, and attached several feedbacks. I think you have the same issue in other files, like delete_node.go, join_node.go and so on. Please update the original code all all files carefully. Then you could comment this pr to seek my further review. Thanks. @starComingup 🌹 🐯 🦁

justadogistaken · 2022-09-08T01:45:43Z

pkg/runtime/k0s/delete_masters.go

+	STEP5: remove bin file such as: kubectl, and remove .kube directory
+	STEP6: remove k0s bin file
+	STEP7: delete node though k8s client
+	*/


I'm wondering if one of the cmds failed, will sealer throw the error for that failed cmd?

I think the error will be thrown by ssh.Async and captured by the caller.

justadogistaken · 2022-09-08T01:49:09Z

pkg/runtime/k0s/delete_masters.go

+	}
+
+	if len(masterIPs) > 0 {
+		hostname, err := k.isHostName(master)


I guess "isHostName" should be "getHostName"?

justadogistaken · 2022-09-08T01:56:22Z

pkg/runtime/k0s/delete_masters.go

+	masterIPs := []net.IP{}
+	for _, ip := range k.cluster.GetMasterIPList() {
+		if !ip.Equal(master) {
+			masterIPs = append(masterIPs, ip)


masterIPs is not used in the below part.
IMO, length of masterIPs plays the role.
So I think we can do like:

masterExist := len(k.cluster.GetMasterIPList()) > 1 if !masterExist { return }

And do we have to judge the condition len(k.cluster.GetMasterIPList()) == 1 and k.cluster.GetMasterIPList()[0].Equal(master) == true. Is this case possible?

I read this code block and I think It did a thing that judge the rest master nodes need greater than one, you code is more clearly and simplify

As for len(k.cluster.GetMasterIPList()) == 1) ,i think it is no need to judge it. Code logic will protect the cluster have a master at least.

justadogistaken · 2022-09-08T01:59:11Z

pkg/runtime/k0s/delete_masters.go

+
+	var name string
+	for _, h := range hosts {
+		if strings.TrimSpace(h) == "" {


You do TrimSpace(h) here. I guess the result from TrimSpace(h) should pass down? hh := strings.ToLower(strings.TrimSpace(h))

Actually, I think we can't pass down value after processed. I don't know how hostname special, but I know field got by k8s client would be it input.

Signed-off-by: starComingup <[email protected]>

allencloud · 2022-09-13T08:15:30Z

apply/processor/utils.go

+	v2 "github.com/sealerio/sealer/types/api/v2"
+)
+
+func RuntimeChoose(rootfs string, cluster *v2.Cluster, config *kubeadm.KubeadmConfig) (runtime.Interface, error) {


@starComingup Here I have a point to discuss with you.

If runtime is a field of details processor instance, or belongs to a processor's function?

If runtime is a field of processor, then RuntimeChoose should be used in a processor's initialization, rather than set runtime in processor's function.

For example, I see

type ScaleProcessor struct { runtime runtime.Interface xxx }

And I see

func (s *ScaleProcessor) PreProcess(cluster *v2.Cluster) error { runTime, err := RuntimeChoose(platform.DefaultMountClusterImageDir(cluster.Name), cluster, s.ClusterFile.GetKubeadmConfig()) if err != nil { return fmt.Errorf("failed to init default runtime: %v", err) } s.Runtime = runTime

I feel quite strange. Why do you RuntimeChoose in ScaleProcessor and set ScaleProcessor's runtime filed, instead of set runtime type in NewScaleProcessor? Initialization in NewScaleProcessor could make code much more clear and more object-oriented.

WDYT?

In addition, please change the function name into ChooseRuntime or GetRuntime rather than RuntimeChoose.

Here I have a point to discuss with you.

If runtime is a field of details processor instance, or belongs to a processor's function?

If runtime is a field of processor, then RuntimeChoose should be used in a processor's initialization, rather than set runtime in processor's function.

For example, I see

The old code set kubernetes.runtime here, so I just make this. But when I coded this, I also felt confused why origin logic set runtime in sub function. I will try to modify it!

allencloud · 2022-09-13T08:23:34Z

apply/processor/utils.go

+		return k0s.NewK0sRuntime(cluster)
+	// Todo case runtime.K3s:
+	default:
+		return kubernetes.NewDefaultRuntime(cluster, config)


I wish to see more thinking details about the default case. From my point of view, if there is no metadata.ClusterRuntime in image rootfs details, we take it as k8s by default. Because in the original implementation of ClusterImage, there is no metadata.ClusterRuntime, and we have to make the new version of sealer compatible with the legacy ClusterImage format. For what is worth, I suggest to add a log to tell end-user that he is using a legacy version, and using new version of sealer and ClusterImage is encouraged.

Otherwise, I think you should add a line of explicit log at Line31 to show the ClusterRuntime info.

I agree with add a cluster runtime info and hint end-user using new version ClusterImage.

allencloud · 2022-09-13T08:27:42Z

pkg/runtime/k0s/reset.go

+	return eg.Wait()
+}
+
+func (k *Runtime) resetMasters(nodes []net.IP) error {


Do we need to use a eg, _ := errgroup.WithContext(context.Background()) in resetMasters as well, just like what you did in resetNodes? @starComingup

I think we can do what you advice, then I will verify it.

allencloud · 2022-09-13T08:30:57Z

pkg/runtime/k0s/runtime.go

@@ -38,48 +41,60 @@ type Runtime struct {
 	RegConfig *registry.Config
 }

+var ForceDelete bool


I have not see any assignment to this variable. Please double check it.

Using this in Line245 confirm force delete.

allencloud · 2022-09-13T08:31:55Z

pkg/runtime/k0s/upgrade.go

+		"k0s start",
+	}
+	var err error
+	for _, ip := range IPs {


Do we need to use a eg, _ := errgroup.WithContext(context.Background()) here?

I don't think it can speed up too much, so I'll keep this logic for now.

allencloud · 2022-09-13T08:32:23Z

pkg/runtime/k0s/upgrade.go

+	return nil
+}
+
+func (k *Runtime) upgradeMasters(IPs []net.IP, binPath string) error {


Do we need to use a eg, _ := errgroup.WithContext(context.Background()) here?

Keep this logic for now, If necessary, I will modify it.

allencloud · 2022-09-13T08:35:14Z

Left some comment, and wait for @starComingup 's feedback and update before merging.

starComingup · 2022-09-15T01:16:25Z

All comment and code updated! Waiting for test.

Signed-off-by: starComingup <[email protected]>

allencloud · 2022-09-15T05:54:47Z

Thanks greatly for your patience during review. @starComingup I think your work could improve sealer quite a lot. Thanks again.

github-actions bot added the test label Sep 5, 2022

justadogistaken reviewed Sep 5, 2022

View reviewed changes

starComingup force-pushed the feature/k0s-runtime branch from b909cc7 to 48cf21c Compare September 5, 2022 07:42

github-actions bot removed the test label Sep 5, 2022

allencloud reviewed Sep 6, 2022

View reviewed changes

starComingup requested a review from allencloud September 8, 2022 01:19

justadogistaken reviewed Sep 8, 2022

View reviewed changes

starComingup force-pushed the feature/k0s-runtime branch from 92dc4a6 to 3edcbcc Compare September 12, 2022 12:40

starComingup added 3 commits September 12, 2022 20:43

feat: add k0s runtime join/delete/reset/upgrade.

053e716

Signed-off-by: starComingup <[email protected]>

resolved: log format, cmd in code, simplify code.

0691113

Signed-off-by: starComingup <[email protected]>

resolved function name confuse, simplify code.

65a56fc

Signed-off-by: starComingup <[email protected]>

starComingup force-pushed the feature/k0s-runtime branch from 3edcbcc to 592a266 Compare September 12, 2022 12:48

add runtime entrance, diff k0s、k8s、k3s。

a487276

Signed-off-by: starComingup <[email protected]>

starComingup force-pushed the feature/k0s-runtime branch from 592a266 to a487276 Compare September 12, 2022 12:58

allencloud reviewed Sep 13, 2022

View reviewed changes

starComingup mentioned this pull request Sep 13, 2022

[BUG] sealer run failed #1702

Closed

starComingup force-pushed the feature/k0s-runtime branch from 1394848 to f7253db Compare September 14, 2022 13:27

resolve conversation and enhanced runtime logic

28bd67e

Signed-off-by: starComingup <[email protected]>

starComingup force-pushed the feature/k0s-runtime branch from f7253db to 28bd67e Compare September 15, 2022 01:34

allencloud approved these changes Sep 15, 2022

View reviewed changes

allencloud merged commit d1a3e78 into sealerio:main Sep 15, 2022

Feature: k0s runtime remain work #1686

Feature: k0s runtime remain work #1686

Conversation

starComingup commented Sep 5, 2022

Describe what this PR does / why we need it

Does this pull request fix one issue?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 5, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

allencloud Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

allencloud commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starComingup Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

allencloud Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

starComingup Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

allencloud commented Sep 13, 2022

starComingup commented Sep 15, 2022

allencloud commented Sep 15, 2022

codecov-commenter commented Sep 5, 2022 •

edited

Loading

allencloud Sep 6, 2022 •

edited

Loading

allencloud Sep 6, 2022 •

edited

Loading

starComingup Sep 14, 2022 •

edited

Loading

allencloud Sep 13, 2022 •

edited

Loading

starComingup Sep 14, 2022 •

edited

Loading