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

spidercoordinator: fix auto fetch podCIDRType #2434

Conversation

cyclinder
Copy link
Collaborator


name: Pull request
about: Tell us about your contribution
labels: ["pr/release/none-required"]


Thanks for contributing !

Before submitting a pull request, make sure you read about our Contribution notice here: https://spidernet-io.github.io/spiderpool/latest/develop/contributing/

What this PR does / why we need it:

  • improve the code side
  • fix unable to auto fetch the podCIDRType

Which issue(s) this PR fixes:

#2433

Special notes for your reviewer:

make sure your commit is signed off

}
}

func setStatus2NoReady(ctx context.Context, copy, c *spiderpoolv2beta1.SpiderCoordinator, cc *CoordinatorController) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个协调器的 handle 代码中,最后会做如下 逻辑

	if reflect.DeepEqual(coordCopy.Status, coord.Status) {
		return nil
	}

因此,此处,应该只需要 设置 copy.Status., 无需额外调用一次 cc.Client.Status() , 以减少一次 api 交互

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

没注意到最后有更新status

@cyclinder cyclinder force-pushed the spidercoordinator/fix_podCIDRType_auto branch from d113992 to 3dfea87 Compare October 18, 2023 11:01
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #2434 (e019315) into main (8894fdd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2434   +/-   ##
=======================================
  Coverage   80.97%   80.97%           
=======================================
  Files          49       49           
  Lines        5251     5251           
=======================================
  Hits         4252     4252           
  Misses        842      842           
  Partials      157      157           
Flag Coverage Δ
unittests 80.97% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@cyclinder cyclinder force-pushed the spidercoordinator/fix_podCIDRType_auto branch 2 times, most recently from bae5c4b to f4c9b2f Compare October 18, 2023 11:19
coordCopy := coord.DeepCopy()

defer func() {
if !reflect.DeepEqual(coordCopy.Status, coord.Status) {
Copy link
Collaborator

@weizhoublue weizhoublue Oct 18, 2023

Choose a reason for hiding this comment

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

这么改 改出 bug 了 , 原来的 代码 中, 最终的 return err 需要 反应 这个 patch 的成功,现在 patch 成功与否,不能 影响 这个回调的返回码。会导致 协调器 后续 重新入队 失败

原来的 代码 挺好的

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

go 的 defer 机制,如果在函数的返回中声明了变量以及类型,真正返回时会将 defer 中赋值完了再返回,举个例子:

package main

import "fmt"

func main() {

	fmt.Println(t1())

}

func t1() (a int) {
	a = 1
	defer func() {
		a = 2
	}()

	a = 3
	return 
}
root@cyclinder:/home/cyclinder/github/spiderpool/cmd/test# go run main.go 
2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这块可以优化下,放在一个地方处理status, 等我参考参考metallb

Copy link
Collaborator

@weizhoublue weizhoublue Oct 18, 2023

Choose a reason for hiding this comment

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

我理解你的场景:你需要的 是 做一次 清除 cidr,但是又要 返回 error
所以,原来 的逻辑 不满足你了

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

还是不要defer,增加代码维护门槛

@cyclinder cyclinder force-pushed the spidercoordinator/fix_podCIDRType_auto branch from f4c9b2f to f6a812d Compare October 18, 2023 13:12
@weizhoublue
Copy link
Collaborator

weizhoublue commented Oct 19, 2023

我觉得在 usage/install/overlay 下的 md ,setup spiderpool 后,得 加入一个步骤,check 这个 cr, 看 cidr 等 符合预期 ,这个是 overlay 搭配场景下的一个重要 验证

@cyclinder cyclinder force-pushed the spidercoordinator/fix_podCIDRType_auto branch 5 times, most recently from ad8bdb2 to c06e179 Compare October 19, 2023 07:41
@weizhoublue
Copy link
Collaborator

E2E 里拉起 calico 和 cilium 的 cidr ,故意和 cluster pod cidr 的不同,用于测试

@cyclinder cyclinder force-pushed the spidercoordinator/fix_podCIDRType_auto branch 4 times, most recently from 3893b79 to 505e02e Compare October 20, 2023 06:41
@cyclinder cyclinder force-pushed the spidercoordinator/fix_podCIDRType_auto branch from 505e02e to cb67590 Compare October 20, 2023 07:20
@cyclinder
Copy link
Collaborator Author

It's OK to merge this.

@@ -34,6 +34,8 @@ export CALICO_VERSION=${CALICO_VERSION:-"v3.25.0"}
export INSTALL_TIME_OUT=${INSTALL_TIME_OUT:-"600s"}
export CALICO_IMAGE_REPO=${CALICO_IMAGE_REPO:-"docker.io"}
export CALICO_AUTODETECTION_METHOD=${CALICO_AUTODETECTION_METHOD:-"kubernetes-internal-ip"}
export CALICO_IPV4POOL_CIDR=${CALICO_CLUSTER_POD_SUBNET_V4:-"10.243.64.0/18"}
Copy link
Collaborator

@weizhoublue weizhoublue Oct 20, 2023

Choose a reason for hiding this comment

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

(1) add CALICO_CLUSTER_POD_SUBNET_V4 to test/Makefile.defs
(2) CILIUM_CLUSTER_POD_SUBNET_V4 has been defined and different from CLUSTER_POD_SUBNET_V4, why the issue did not happen in cilium cluster before ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why the issue did not happen in cilium cluster before ?

Currently, E2E does not test pods the connectivity of the underlay NIC and the overlay NIC, so even if the overlay CIDRType is wrong, e2e also can pass.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ty-dc are there any E2E planned for this ?

Copy link
Collaborator

@ty-dc ty-dc Oct 23, 2023

Choose a reason for hiding this comment

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

@ty-dc are there any E2E planned for this ?

Use kdcotor for connectivity testing, but it does not test the connectivity of the pod's underlying NIC and overlay NIC. To do this, add an issue and resolve it as soon as possible.

@cyclinder cyclinder force-pushed the spidercoordinator/fix_podCIDRType_auto branch from cb67590 to 9c31bab Compare October 23, 2023 02:07
@cyclinder cyclinder force-pushed the spidercoordinator/fix_podCIDRType_auto branch from 9c31bab to e019315 Compare October 23, 2023 12:41
@weizhoublue weizhoublue merged commit 790c5f5 into spidernet-io:main Oct 24, 2023
38 checks passed
This was referenced Oct 30, 2023
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.

3 participants