refactor: the node convert/revert feature #206

Peeknut · 2021-01-27T05:17:49Z

Ⅰ. Describe what this PR does

1.Refactor the node convert/revert feature by golang.
2.Add subcommand yurtctl convert edgenode and yurtctl revert edgenode, so that single node could convert/revert Kubernetes edgenode to an Openyurt edgenode.

Ⅱ. Does this pull request fix one issue?

fixes #190

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

make test
make build
cp _output/bin/yurtctl /bin/
yurtctl convert
yurtctl revert
yurtctl convert edgenode
yurtctl revert edgenode

Ⅴ. Special notes for reviews

CLAassistant · 2021-01-27T05:17:55Z

All committers have signed the CLA.

charleszheng44

Thanks for submitting the PR! It looks good except for some minor issues. If these changes are applied, do we need to change the way of running yurtctl convert/revert? If so, would you also update the tutorial? Thanks!

charleszheng44 · 2021-01-27T05:54:57Z

config/yurtctl-servant/Dockerfile

@@ -1,4 +1 @@
-FROM alpine:3.8


why use ubuntu instead of alpine?

When the pod Yurtctl-servant-convert runs the command nsenter -t 1 -m -u -n -i /bin/yurtctl convert edgenode --yurthub-image {{.yurthub_image}} --join-token {{.joinToken}} --pod-manifest-path {{.pod_manifest_path}} , command nsenter in alpine will parse --yurthub-image into the parameters of nsenter. It will not have this problem with ubuntu.

Thanks for the explanation. Would you try nsenter -t 1 -m -u -n -i -- /bin/yurtctl convert edgenode --yurthub-image {{.yurthub_image}} --join-token {{.joinToken}} --pod-manifest-path {{.pod_manifest_path}} in alpine? Let's use '--' to tell nsenter to stop option parsing. As the size of the alpine is smaller than the ubuntu, using it can help us reduce the network throughput when setting up a large cluster.

Thanks！I test it and it works！

charleszheng44 · 2021-01-27T05:59:22Z

pkg/yurtctl/cmd/convert/edgenode.go

+	openyurtDir     string
+}
+
+func NewConvertEdgeNodeOptions() *ConvertEdgeNodeOptions {


Could you add GoDoc comment for the function and other exposed functions?

charleszheng44 · 2021-01-27T06:01:46Z

pkg/yurtctl/cmd/convert/edgenode.go

+		},
+	}
+
+	cmd.Flags().String("join-token", "", "the join token.")


The token used by yurthub for joining the cluster.

charleszheng44 · 2021-01-27T06:03:39Z

pkg/yurtctl/cmd/convert/edgenode.go

@@ -0,0 +1,276 @@
+package convert


Please add the license.

How to add the license? Make verify?

The OpenSource License : )

/* Copyright 2021 The OpenYurt Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */

charleszheng44 · 2021-01-27T06:04:03Z

pkg/yurtctl/cmd/revert/edgenode.go

@@ -0,0 +1,182 @@
+package revert


Please add the license.

charleszheng44 · 2021-01-27T06:04:51Z

pkg/yurtctl/util/edgenode/exec.go

@@ -0,0 +1,76 @@
+package edgenode


Please add the license.

charleszheng44 · 2021-01-27T06:09:18Z

pkg/yurtctl/util/edgenode/exec.go

+
+// Exec run command and exit formatted error, callers can print err directly
+// Any running error or non-zero exitcode is consider as error
+func (cmd *Command) Exec() error {


why not use os/exec package?

The struct Command contains all the information about cmd running and can be used in other places. If wants to be simpler, os/exec can be uses here.

charleszheng44 · 2021-01-27T06:09:35Z

pkg/yurtctl/util/edgenode/util.go

@@ -0,0 +1,105 @@
+package edgenode


Please add the license.

charleszheng44 · 2021-01-27T06:11:14Z

pkg/yurtctl/util/edgenode/util.go

+	}
+	if contents == nil {
+		return "", fmt.Errorf("no matching string %s in file %s", regularExpression, filename)
+	} else if len(contents) > 1 {


else if len(contents) > 1 {} => if len(contents) > 1 {}

charleszheng44 · 2021-01-27T06:13:12Z

pkg/yurtctl/util/kubernetes/util.go

 	var wg sync.WaitGroup
+	var servantJobTemplate string


servantJobTemplate := constants.ConvertServantJobTemplate if !convert { servantJobTemplate = constants.RevertServantJobTemplate }

rambohe-ch · 2021-01-27T07:00:19Z

pkg/yurtctl/cmd/revert/edgenode.go

+		return err
+	}
+	klog.Infof("found backup file %s, will use it to revert the node", kubeletSvcBk)
+	err = os.Rename(kubeletSvcBk, r.kubeletSvcPath)


maybe we need delete the converted kubeletSvcPath file before rename the backup file?

The effect of os.Rename() is similar to mv, it will overwrite original file. So the converted kubeletSvcPath file will be deleted at the same time. : )

ok, i got it

rambohe-ch · 2021-01-27T07:11:07Z

pkg/yurtctl/constants/constants.go

+        - name: NODE_NAME
+          valueFrom:
+            fieldRef:
+              fieldPath: spec.nodeName
 `


env STATIC_POD_PATH is missed

yurtctl convert provides the value of STATIC_POD_PATH：

if err = kubeutil.RunServantJobs(co.clientSet, map[string]string{
"provider": string(co.Provider),
"action": "convert",
"yurtctl_servant_image": co.YurctlServantImage,
"yurthub_image": co.YurhubImage,
"joinToken": joinToken,
"pod_manifest_path": co.PodMainfestPath,
}, edgeNodeNames, true);

however, yurtctl revert not provides it:

if err = kubeutil.RunServantJobs(ro.clientSet,
map[string]string{
"action": "revert",
"yurtctl_servant_image": ro.YurtctlServantImage,
},
edgeNodeNames, false);

so I delete the env STATIC_POD_PATH in RevertServantJobTemplate

yeah, must had missed STATIC_POD_PATH var for yurtctl revert command, how ablout add STATIC_POD_PATH for yurtctl revert cmd?

rambohe-ch · 2021-01-27T07:13:24Z

pkg/yurtctl/util/edgenode/util.go

+	return content
+}
+
+func GetNodeName() (string, error) {


how about use env NODE_NAME in convert and revert command instead of new GetNodeName() function?

Does NODE_NAME refer to the env HOSTNAME?
If so, kubelet startup parameters --hostname-override can be used to configure the host name of the node displayed in the cluster, so the node name in cluster can be different from env HOSTNAME. And it need to get the node name in cluster accurately, because we need to get node resources through node name later.
So maybe it's better to use GetNodeName() function?

using NODE_NAME means we can add the following setting to the pod spec

... env: - name: NODE_NAME valueFrom: fieldRef: fieldPath: spec.nodeName ...

But yes, I think we should try to get the correct nodename from the service configuration directly, if --hostname-override can be used to overwrite the nodename.

@Peeknut as @charleszheng44 had said, env NODE_NAME is set in ServantJobTemplate by spec.nodeName field, so in spite of --hostname-override is set or not, we can get the node name accurately by env NODE_NAME .

That's right. For yurtctl convert/revert, we can directly use NODE_NAME. However, for directly calling yurtctl convert/revert edgenode, NODE_NAME may not be set, we need to read the configuration file to get it. Both approaches may be required. Maybe I should add steps to read env NODE_NAME in GetNodeName() function?

rambohe-ch · 2021-01-27T07:15:48Z

pkg/yurtctl/cmd/revert/edgenode.go

+	isEdgeNode, ok := node.Labels[projectinfo.GetEdgeWorkerLabelKey()]
+	if ok && isEdgeNode == "true" {
+		// 2.1. check the state of worker nodes
+		_, condition := nodeutil.GetNodeCondition(&node.Status, v1.NodeReady)


maybe we need add node ready status check at the begin of edgenode convert and revert command.

yurtctl revert has node ready status check at the beginning of the command.

yurctl revert edgenode also has node status check. Before node status check, checking the server version and getting node resource are necessary. Does the check time need to be changed?

We can check the node status before conversion/reversion, but the node can still fail when the conversion/reversion is ongoing. To eliminate this, we need to add a CRD/Controller to manage the OpenYurt cluster state, but that can be another new feature. I think we can keep the concurrent implementation just for now.

+1 keep the current implementation

rambohe-ch · 2021-01-27T07:17:47Z

@Peeknut Please merge all commits in this pr(now has 3 commits) into one commit.

Peeknut · 2021-01-27T08:27:26Z

@Peeknut Please merge all commits in this pr(now has 3 commits) into one commit.

OK!

GsssC · 2021-01-27T09:06:29Z

pkg/yurtctl/cmd/convert/edgenode.go

+	}
+	c.PodMainfestPath = podMainfestPath
+
+	c.clientSet, err = kubeutil.GenClientSet(flags)


if there is no kubeconfig in all flag, KUBECONFIG, ~/.kube/config, it will return err.

@rambohe-ch , how about use /etc/kubernetes/kubelet.conf if err occurs? kubelet.conf has enougth authority to set label for current edge node.

+1 for using /etc/kubernetes/kubelet.conf as the default.

@GsssC how about use InClusterConfig at the edge node? plus add a serviceaccount and rabc for servant pod.

yurtctl convert/revert edgenode could be used directly on the single node. In this case, we can't use InClusterConfig at the edge node because it is not running in a pod environment. Maybe using /etc/kubernetes/kubelet.conf as the default is better?

charleszheng44 · 2021-01-27T20:29:31Z

pkg/yurtctl/cmd/convert/edgenode.go

@@ -0,0 +1,276 @@
+package convert


The OpenSource License : )

/* Copyright 2021 The OpenYurt Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. */

charleszheng44 · 2021-01-27T20:32:38Z

pkg/yurtctl/cmd/convert/edgenode.go

+	}
+	c.PodMainfestPath = podMainfestPath
+
+	c.clientSet, err = kubeutil.GenClientSet(flags)


+1 for using /etc/kubernetes/kubelet.conf as the default.

charleszheng44 · 2021-01-27T20:47:34Z

pkg/yurtctl/cmd/revert/edgenode.go

+	isEdgeNode, ok := node.Labels[projectinfo.GetEdgeWorkerLabelKey()]
+	if ok && isEdgeNode == "true" {
+		// 2.1. check the state of worker nodes
+		_, condition := nodeutil.GetNodeCondition(&node.Status, v1.NodeReady)


We can check the node status before conversion/reversion, but the node can still fail when the conversion/reversion is ongoing. To eliminate this, we need to add a CRD/Controller to manage the OpenYurt cluster state, but that can be another new feature. I think we can keep the concurrent implementation just for now.

charleszheng44 · 2021-01-27T20:57:36Z

pkg/yurtctl/util/edgenode/util.go

+	return content
+}
+
+func GetNodeName() (string, error) {


using NODE_NAME means we can add the following setting to the pod spec

... env: - name: NODE_NAME valueFrom: fieldRef: fieldPath: spec.nodeName ...

But yes, I think we should try to get the correct nodename from the service configuration directly, if --hostname-override can be used to overwrite the nodename.

Peeknut · 2021-01-29T03:04:02Z

@rambohe-ch @charleszheng44 @GsssC
PTAL

rambohe-ch · 2021-01-29T03:15:49Z

pkg/yurtctl/cmd/convert/edgenode.go

+func NewConvertEdgeNodeCmd() *cobra.Command {
+	c := NewConvertEdgeNodeOptions()
+	cmd := &cobra.Command{
+		Use:   "edgenode",


i think we should specify node name for this subcommand, like yurtctl convert/revert edgenode node-name --xxx=xxx, so this subcommand can be worked at everywhere with kubeconfig.

And this subcommand can work only on the edge node that user want to convert or revert in current design, I think it's not user-friendly.

so this sub command can need run servant pod to exec convert or revert command.

Peeknut · 2021-02-02T06:09:00Z

PTAL @rambohe-ch @charleszheng44

rambohe-ch · 2021-02-04T10:04:29Z

pkg/yurtctl/cmd/convert/edgenode.go

+			}
+		}
+
+		// 2.2. check the state of worker nodes


not check worker nodes here, so how about check the state of EdgeNodes

+1 for EdgeNodes

charleszheng44

LGTM

charleszheng44 · 2021-02-05T02:18:24Z

pkg/yurtctl/cmd/convert/edgenode.go

+			}
+		}
+
+		// 2.2. check the state of worker nodes


+1 for EdgeNodes

pkg/yurtctl/cmd/convert/edgenode.go

pkg/yurtctl/cmd/revert/edgenode.go

rambohe-ch · 2021-02-08T02:30:56Z

LGTM

refactor: the node convert/revert feature

charleszheng44 suggested changes Jan 27, 2021

View reviewed changes

rambohe-ch reviewed Jan 27, 2021

View reviewed changes

rambohe-ch mentioned this pull request Jan 27, 2021

[feature request] Command convert of yurtctl support param for kubelet service kubeadm conf #203

Closed

GsssC reviewed Jan 27, 2021

View reviewed changes

charleszheng44 suggested changes Jan 27, 2021

View reviewed changes

Peeknut force-pushed the refactor branch from 73f8e34 to 8b352cf Compare January 29, 2021 02:31

rambohe-ch reviewed Jan 29, 2021

View reviewed changes

Peeknut force-pushed the refactor branch from 8b352cf to 455ef3c Compare February 2, 2021 06:03

rambohe-ch reviewed Feb 4, 2021

View reviewed changes

charleszheng44 approved these changes Feb 5, 2021

View reviewed changes

pkg/yurtctl/cmd/convert/edgenode.go Outdated

}

}

// 2.2. check the state of worker nodes

Copy link

Member

charleszheng44 Feb 5, 2021

Choose a reason for hiding this comment

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

+1 for EdgeNodes

Peeknut force-pushed the refactor branch from 455ef3c to f015df0 Compare February 6, 2021 08:00

rambohe-ch reviewed Feb 7, 2021

View reviewed changes

pkg/yurtctl/cmd/convert/edgenode.go Show resolved Hide resolved

rambohe-ch reviewed Feb 7, 2021

View reviewed changes

pkg/yurtctl/cmd/convert/edgenode.go Show resolved Hide resolved

rambohe-ch reviewed Feb 7, 2021

View reviewed changes

pkg/yurtctl/cmd/revert/edgenode.go Show resolved Hide resolved

rambohe-ch reviewed Feb 7, 2021

View reviewed changes

pkg/yurtctl/cmd/revert/edgenode.go Show resolved Hide resolved

refactor the node convert/revert feature

851d8f0

Peeknut force-pushed the refactor branch from f015df0 to 851d8f0 Compare February 7, 2021 13:38

rambohe-ch merged commit 074711d into openyurtio:master Feb 8, 2021

Peeknut deleted the refactor branch February 25, 2021 11:36

Peeknut mentioned this pull request Jun 20, 2021

[REQUEST Membership]: New membership for Peeknut: reviewer of openyurtio/openyurt #362

Closed

MrGirl pushed a commit to MrGirl/openyurt that referenced this pull request Mar 29, 2022

Merge pull request openyurtio#206 from Peeknut/refactor

09063d4

refactor: the node convert/revert feature

refactor: the node convert/revert feature #206

refactor: the node convert/revert feature #206

Conversation

Peeknut commented Jan 27, 2021

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

CLAassistant commented Jan 27, 2021 • edited Loading

charleszheng44 left a 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

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

Peeknut Jan 27, 2021 • 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

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

rambohe-ch commented Jan 27, 2021

Peeknut commented Jan 27, 2021

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

Peeknut commented Jan 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peeknut commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleszheng44 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rambohe-ch commented Feb 8, 2021

CLAassistant commented Jan 27, 2021 •

edited

Loading

Peeknut Jan 27, 2021 •

edited

Loading