support taint cluster node through clusterfile #1894

Stevent-fei · 2022-11-28T01:49:11Z

Describe what this PR does / why we need it

add taint to cluster type
ClusterFile supports configuring node taint support taint cluster node through clusterfile #1889

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

codecov-commenter · 2022-11-28T01:53:29Z

Codecov Report

Base: 21.38% // Head: 21.56% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (3a88984) compared to base (3897332).
Patch coverage: 35.29% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1894      +/-   ##
==========================================
+ Coverage   21.38%   21.56%   +0.18%     
==========================================
  Files          78       79       +1     
  Lines        7300     7401     +101     
==========================================
+ Hits         1561     1596      +35     
- Misses       5531     5591      +60     
- Partials      208      214       +6

Impacted Files	Coverage Δ
pkg/cluster-runtime/installer.go	`0.00% <0.00%> (ø)`
pkg/cluster-runtime/scale.go	`0.00% <0.00%> (ø)`
pkg/infradriver/ssh_infradriver.go	`40.00% <35.29%> (-0.49%)`	⬇️
pkg/infradriver/utils.go	`61.22% <61.22%> (ø)`

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.

starnop · 2022-11-30T03:13:54Z

cmd/sealer/cmd/cluster/apply.go

@@ -175,7 +175,9 @@ func createNewCluster(clusterImageName string, infraDriver infradriver.InfraDriv
 	if applyMode == common.ApplyModeLoadImage {
 		logrus.Infof("start to apply with mode(%s)", applyMode)

-		if err = distributor.DistributeRegistry(infraDriver.GetHostIPList()[0], infraDriver.GetClusterRootfsPath()); err != nil {
+		cluster := cf.GetCluster()


what's the difference between cf.GetCluster().GetMaster0IP() and infraDriver.GetHostIPListByRole(common.MASTER)[0]

starnop · 2022-11-30T03:16:11Z

How about splitting this PR into three PRs, we should try to ensure that a PR only does one thing, which is convenient to be reviewed and merged quickly.

kakaZhou719 · 2022-12-02T02:59:49Z

pkg/cluster-runtime/taint.go

+}
+
+func newTaintStruct(key, value, effect string) v1.Taint {
+	return v1.Taint{Key: key, Value: value, Effect: v1.TaintEffect(effect)}


no need new struct Taint to do this

kakaZhou719 · 2022-12-02T03:00:06Z

pkg/cluster-runtime/taint.go

+// FormatData key1=value1:NoSchedule;key1=value1:NoSchedule-;
+//key1:NoSchedule;key1:NoSchedule-;
+//key1=:NoSchedule-;key1=value1:NoSchedule
+func (l *Taint) FormatData(ip net.IP, data string) error {


do FormatData in infra driver

kakaZhou719 · 2022-12-02T03:01:18Z

pkg/cluster-runtime/taint.go

+}
+
+// UpdateTaints return nil mean's needn't update taint
+func (l *Taint) UpdateTaints(taints []v1.Taint, ip string) []v1.Taint {


update taints should be a class functions of installer.

kakaZhou719 · 2022-12-02T03:02:59Z

pkg/cluster-runtime/installer.go

@@ -188,6 +188,19 @@ func (i *Installer) Install() error {
 		return err
 	}

+	for _, ip := range all {


what if the new joined node need to do some taint issues ? if so ,we also need to do it in join

kakaZhou719 · 2022-12-02T03:05:41Z

pkg/cluster-runtime/taint.go

+}
+
+func UpdateTaint(newTaint *Taint, ip net.IP) error {
+	k8sClient, err := k8s.NewK8sClient()


use runtime driver to do update

kakaZhou719 · 2022-12-05T07:59:23Z

pkg/infradriver/infradriver.go

@@ -23,6 +23,9 @@ import (
 // InfraDriver treat the entire cluster as an operating system kernel,
 // interface function here is the target system call.
 type InfraDriver interface {
+	// GetHostTaint return host taint
+	GetHostTaint(host net.IP) []string


GetHostTaints(host net.IP)[]Taint

we need the formatted data from raw sting list to taint struct.

kakaZhou719 · 2022-12-05T08:00:37Z

pkg/infradriver/utils.go

+// FormatData key1=value1:NoSchedule;key1=value1:NoSchedule-;
+//key1:NoSchedule;key1:NoSchedule-;
+//key1=:NoSchedule-;key1=value1:NoSchedule
+func (t *Taint) FormatData(ip net.IP, data string) error {


FormatData should be a utils func , FormatData(data []string)([]taint,error)

kakaZhou719 · 2022-12-05T08:01:14Z

pkg/infradriver/utils.go

+	return node, nil
+}
+
+func NewCreateTaints() *Taint {


no need this method

kakaZhou719 · 2022-12-05T08:01:55Z

pkg/infradriver/utils.go

+}
+
+// UpdateTaints return nil mean's needn't update taint
+func (t *Taint) UpdateTaints(taints []k8sv1.Taint, ip string) []k8sv1.Taint {


UpdateTaints and UpdateNodeTaint should in installer struct.

Stevent-fei · 2022-12-05T10:25:28Z

kakaZhou719 · 2022-12-06T01:57:02Z

pkg/infradriver/ssh_infradriver.go

@@ -57,6 +59,16 @@ func mergeList(hostEnv, globalEnv map[string]interface{}) map[string]interface{}
 	return hostEnv
 }

+func conversionTaints(Taints []string) []k8sv1.Taint {


convertTaints(raw []string) []k8sv1.Taint

kakaZhou719 · 2022-12-06T01:57:40Z

pkg/infradriver/ssh_infradriver.go

@@ -57,6 +59,16 @@ func mergeList(hostEnv, globalEnv map[string]interface{}) map[string]interface{}
 	return hostEnv
 }

+func conversionTaints(Taints []string) []k8sv1.Taint {
+	for _, taint := range Taints {


for _, elem := range raw{}

kakaZhou719 · 2022-12-06T01:59:22Z

pkg/infradriver/ssh_infradriver.go

@@ -57,6 +59,16 @@ func mergeList(hostEnv, globalEnv map[string]interface{}) map[string]interface{}
 	return hostEnv
 }

+func conversionTaints(Taints []string) []k8sv1.Taint {
+	for _, taint := range Taints {
+		data, err := FormatData(taint)


should return err if format failed.

kakaZhou719 · 2022-12-06T02:00:24Z

pkg/infradriver/ssh_infradriver.go

@@ -57,6 +59,16 @@ func mergeList(hostEnv, globalEnv map[string]interface{}) map[string]interface{}
 	return hostEnv
 }

+func conversionTaints(Taints []string) []k8sv1.Taint {


we need a []k8sv1.Taint type to return

kakaZhou719 · 2022-12-06T02:01:00Z

pkg/infradriver/utils.go

+
+	for _, v := range items {
+		v = strings.TrimSpace(v)
+		if strings.HasPrefix(v, "#") || v == "" {


no need this line

kakaZhou719 · 2022-12-06T02:01:17Z

pkg/infradriver/utils.go

+		if strings.HasPrefix(v, "#") || v == "" {
+			continue
+		}
+		if strings.Contains(v, EqualSymbol) && !strings.Contains(v, EqualSymbol+ColonSymbol) {


use switch case to do it.

kakaZhou719 · 2022-12-06T02:03:46Z

pkg/cluster-runtime/installer.go

+	}
+
+	for _, node = range nodeList.Items {
+		for _, ip := range hosts {


bug: hosts is a subset for nodelist ,so we need to find the target node by host ip to do taint action

kakaZhou719 · 2022-12-06T02:06:36Z

pkg/cluster-runtime/installer.go

+			if taints != nil {
+				node.Spec.Taints = taints
+			}
+			if err := driver.Update(context.TODO(), &node); err != nil {


if taints != nil {do update node}

kakaZhou719 · 2022-12-06T08:41:56Z

pkg/infradriver/utils.go

+
+// FormatData process data in the specified format
+//eg: key1=value1:NoSchedule;key1:NoSchedule;key1=value1:NoSchedule
+func formatData(data string) (k8sv1.Taint, error) {


we need UT test for this func.

kakaZhou719 · 2022-12-06T08:44:56Z

pkg/cluster-runtime/installer.go

+
+	for _, ip := range hosts {
+		taints := i.infraDriver.GetHostTaints(ip)
+		if taints != nil {


if len(taints)==0{continue }

kakaZhou719 · 2022-12-06T08:46:49Z

pkg/cluster-runtime/installer.go

+			if k8snode, ok = nodeTaint[ip.String()]; ok {
+				k8snode.Spec.Taints = taints
+			}
+			if err := driver.Update(context.TODO(), &k8snode); err != nil {


bug: should update node if node ip in nodeTaint. Otherwise , no need to do update.

VinceCui · 2022-12-07T03:39:27Z

pkg/infradriver/utils_test.go

+)
+
+//key1=value1:NoSchedule
+func TestFormatData(t *testing.T) {


test case too few.

value can be empty.

I have added more ut.

VinceCui · 2022-12-07T03:43:35Z

pkg/cluster-runtime/installer.go

+			newNode := k8snode.DeepCopy()
+			newNode.Spec.Taints = taints
+			newNode.SetResourceVersion("")
+			if err := driver.Update(context.TODO(), newNode); err != nil {


Update should retry, if some other agent, like kubelet, update the node object at the same time, this update will fail.

VinceCui

LGTM

github-actions bot added the Clusterfile label Nov 28, 2022

Stevent-fei force-pushed the support_add_node_taint branch from ebb6e79 to efb15a0 Compare November 28, 2022 03:56

Stevent-fei changed the title ~~[WIP]ClusterFile supports configuring node taint~~ ClusterFile supports configuring node taint Nov 28, 2022

Stevent-fei linked an issue Nov 28, 2022 that may be closed by this pull request

support taint cluster node through clusterfile #1889

Closed

Stevent-fei changed the title ~~ClusterFile supports configuring node taint~~ Add Clusterfile Configure the taint function Nov 28, 2022

Stevent-fei force-pushed the support_add_node_taint branch from 72dc275 to db2822d Compare November 28, 2022 05:47

starnop reviewed Nov 30, 2022

View reviewed changes

Stevent-fei force-pushed the support_add_node_taint branch from db2822d to 2fce24a Compare December 1, 2022 02:39

kakaZhou719 reviewed Dec 2, 2022

View reviewed changes

Stevent-fei changed the title ~~Add Clusterfile Configure the taint function~~ support taint cluster node through clusterfile Dec 2, 2022

Stevent-fei force-pushed the support_add_node_taint branch 2 times, most recently from e69bdf3 to f87dfe2 Compare December 2, 2022 10:58

Stevent-fei changed the title ~~support taint cluster node through clusterfile~~ [WIP]support taint cluster node through clusterfile Dec 3, 2022

Stevent-fei force-pushed the support_add_node_taint branch 3 times, most recently from d3ccb0b to d35a665 Compare December 3, 2022 10:18

Stevent-fei changed the title ~~[WIP]support taint cluster node through clusterfile~~ support taint cluster node through clusterfile Dec 3, 2022

kakaZhou719 reviewed Dec 5, 2022

View reviewed changes

Stevent-fei force-pushed the support_add_node_taint branch from 6adcb7c to e1e5f5f Compare December 5, 2022 10:24

Stevent-fei requested review from starnop and kakaZhou719 December 5, 2022 10:25

kakaZhou719 reviewed Dec 6, 2022

View reviewed changes

Stevent-fei force-pushed the support_add_node_taint branch 2 times, most recently from 1773a79 to 36bf87d Compare December 6, 2022 03:34

kakaZhou719 reviewed Dec 6, 2022

View reviewed changes

github-actions bot added the test label Dec 6, 2022

Stevent-fei force-pushed the support_add_node_taint branch 2 times, most recently from 28b4885 to 4ae01d0 Compare December 7, 2022 01:58

VinceCui reviewed Dec 7, 2022

View reviewed changes

Stevent-fei force-pushed the support_add_node_taint branch 4 times, most recently from ee3fb4c to 8971351 Compare December 9, 2022 09:33

Stevent-fei force-pushed the support_add_node_taint branch from 8971351 to 3206fd0 Compare December 15, 2022 09:18

VinceCui approved these changes Dec 15, 2022

View reviewed changes

resolve conflict

3a88984

Stevent-fei force-pushed the support_add_node_taint branch from 1ff03c0 to 3a88984 Compare December 15, 2022 09:24

VinceCui merged commit 9751ea8 into sealerio:main Dec 15, 2022

jsparter pushed a commit to jsparter/sealer that referenced this pull request Dec 16, 2022

resolve conflict (sealerio#1894)

bacb12e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support taint cluster node through clusterfile #1894

support taint cluster node through clusterfile #1894

Stevent-fei commented Nov 28, 2022 •

edited

Loading

codecov-commenter commented Nov 28, 2022 •

edited

Loading

starnop Nov 30, 2022

starnop commented Nov 30, 2022

kakaZhou719 Dec 2, 2022

kakaZhou719 Dec 2, 2022

kakaZhou719 Dec 2, 2022

kakaZhou719 Dec 2, 2022

kakaZhou719 Dec 2, 2022

kakaZhou719 Dec 5, 2022

kakaZhou719 Dec 5, 2022

kakaZhou719 Dec 5, 2022

kakaZhou719 Dec 5, 2022

kakaZhou719 Dec 5, 2022

Stevent-fei commented Dec 5, 2022

kakaZhou719 Dec 6, 2022

kakaZhou719 Dec 6, 2022

kakaZhou719 Dec 6, 2022

kakaZhou719 Dec 6, 2022

kakaZhou719 Dec 6, 2022

kakaZhou719 Dec 6, 2022

kakaZhou719 Dec 6, 2022 •

edited

Loading

kakaZhou719 Dec 6, 2022

kakaZhou719 Dec 6, 2022

kakaZhou719 Dec 6, 2022

kakaZhou719 Dec 6, 2022

VinceCui Dec 7, 2022

Stevent-fei Dec 7, 2022

VinceCui Dec 7, 2022

VinceCui left a comment

support taint cluster node through clusterfile #1894

support taint cluster node through clusterfile #1894

Conversation

Stevent-fei commented Nov 28, 2022 • edited Loading

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

codecov-commenter commented Nov 28, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

starnop commented Nov 30, 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

Stevent-fei commented Dec 5, 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

kakaZhou719 Dec 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceCui left a comment

Choose a reason for hiding this comment

Stevent-fei commented Nov 28, 2022 •

edited

Loading

codecov-commenter commented Nov 28, 2022 •

edited

Loading

kakaZhou719 Dec 6, 2022 •

edited

Loading