fix: fail to get podCIDR and serviceCIDR #4366

0x0034 · 2024-12-08T11:16:49Z

Thanks for contributing!

Notice:

unite test or E2E test
do not forget essential code comment and log
document for the PR
release note label
"release/none"
"release/bug"
"release/feature"
read about Contribution notice: https://spidernet-io.github.io/spiderpool/latest/develop/contributing/

What issue(s) does this PR fix:

Fixes #

Special notes for your reviewer:

0x0034 · 2024-12-09T02:31:03Z

原先的想法是使用 kubeadm 自带的 ClusterConfiguration 进行反序列化, 但是对于一些较低版本的 kubernetes, 需要兼容多个版本 v1beta1, v1beta2, v1beta3, v1beta4, 所以还是采取了字符串搜索的模式, 仅仅优化了查询逻辑, 和查询失败的降级逻辑

ty-dc · 2024-12-09T02:33:31Z

pkg/coordinatormanager/coordinator_informer.go

-		k8sPodCIDR, k8sServiceCIDR = ExtractK8sCIDRFromKubeadmConfigMap(&cm)
-		logger.Sugar().Infof("kubeadm-config configMap k8sPodCIDR %v, k8sServiceCIDR %v", k8sPodCIDR, k8sServiceCIDR)
-	} else {
+	// 尝试从 kubeadm-config ConfigMap 获取 ClusterCIDR


代码注释统一风格，更应该是英文表达。

cyclinder

感谢优化，代码 looks good to me, 几点小小的建议：

解决下 ci 的问题
可以为 ExtractK8sCIDRFromKubeadmConfigMap 函数添加一个单元测试函数

cyclinder · 2024-12-09T01:55:19Z

pkg/coordinatormanager/coordinator_informer.go

-		k8sPodCIDR, k8sServiceCIDR = ExtractK8sCIDRFromKubeadmConfigMap(&cm)
-		logger.Sugar().Infof("kubeadm-config configMap k8sPodCIDR %v, k8sServiceCIDR %v", k8sPodCIDR, k8sServiceCIDR)
-	} else {
+	// 尝试从 kubeadm-config ConfigMap 获取 ClusterCIDR


可以改下英文注释

cyclinder · 2024-12-09T02:35:00Z

原先的想法是使用 kubeadm 自带的 ClusterConfiguration 进行反序列化, 但是对于一些较低版本的 kubernetes, 需要兼容多个版本 v1beta1, v1beta2, v1beta3, v1beta4, 所以还是采取了字符串搜索的模式, 仅仅优化了查询逻辑, 和查询失败的降级逻辑

是的，这里使用字符串搜索的模式即可

cyclinder · 2024-12-09T02:35:26Z

另外请使用 git commit --amend -s 签名下你的 commit

codecov · 2024-12-09T02:40:56Z

Codecov Report

Attention: Patch coverage is 0% with 40 lines in your changes missing coverage. Please review.

Project coverage is 6.80%. Comparing base (7fd26d8) to head (5a5a2c1).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/coordinatormanager/coordinator_informer.go	0.00%	40 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (7fd26d8) and HEAD (5a5a2c1). Click for more details.

HEAD has 3 uploads less than BASE

Flag BASE (7fd26d8) HEAD (5a5a2c1)

unittests 4 1

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #4366       +/-   ##
==========================================
- Coverage   79.26%   6.80%   -72.46%     
==========================================
  Files          54      59        +5     
  Lines        6283    7174      +891     
==========================================
- Hits         4980     488     -4492     
- Misses       1108    6651     +5543     
+ Partials      195      35      -160

Flag	Coverage Δ
unittests	`6.80% <0.00%> (-72.46%)`	⬇️

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

Files with missing lines	Coverage Δ
pkg/coordinatormanager/coordinator_informer.go	`0.00% <0.00%> (ø)`

... and 55 files with indirect coverage changes

0x0034 · 2024-12-09T06:09:08Z

感谢优化，代码 looks good to me, 几点小小的建议：

解决下 ci 的问题

可以为 ExtractK8sCIDRFromKubeadmConfigMap 函数添加一个单元测试函数

为 ExtractK8sCIDRFromKubeadmConfigMap 函数添加一个单元测试函数
注释修改为中文
git commit --amend -s 签名
CI 问题

CI 问题我不是很清楚具体是啥问题, 或者可以告诉我需要解决啥嘛

ty-dc · 2024-12-09T06:26:57Z

CI 问题我不是很清楚具体是啥问题, 或者可以告诉我需要解决啥嘛

Error: ./spidercoordinator_test.go:467:28: assignment mismatch: 2 variables but coordinatormanager.ExtractK8sCIDRFromKubeadmConfigMap returns 3 values

变更了 ExtractK8sCIDRFromKubeadmConfigMap，导致返回值不匹配。应该同步更改一下 e2e case 。

0x0034 · 2024-12-09T07:19:54Z

我对于Ginkgo 不是很熟悉, 尝试往 e2e 测试中加入了一些 case , 不知道这么写是否符合规范

cyclinder · 2024-12-09T08:47:25Z

test/e2e/spidercoordinator/spidercoordinator_test.go

@@ -389,7 +390,77 @@ var _ = Describe("SpiderCoordinator", Label("spidercoordinator", "overlay"), Ser
 		var masterNode string
 		var err error

+		var clusterConfigurationInOneLineCm *corev1.ConfigMap


测试函数本身就在函数文件内闭环即可， test 放的是 e2e 测试，更多是端到端的测试。所以把这部分挪到 pkg/coordinatormanager/coordinator_informer_test.go 就可

cyclinder · 2024-12-09T08:48:36Z

pkg/coordinatormanager/coordinator_informer_test.go

+		cm *corev1.ConfigMap
+	}
+
+	clusterConfigurationInOneLineJson := `


这个 string 可以简化和输出友好一下，目前可读性有点差

ty-dc · 2024-12-09T09:18:52Z

test/e2e/spidercoordinator/spidercoordinator_test.go

@@ -464,7 +464,7 @@ var _ = Describe("SpiderCoordinator", Label("spidercoordinator", "overlay"), Ser
 		It("Prioritize getting ClusterCIDR from kubeadm-config", Label("V00009"), func() {
 			GinkgoWriter.Printf("podCIDR and serviceCIDR from spidercoordinator: %v,%v\n", spc.Status.OverlayPodCIDR, spc.Status.ServiceCIDR)

-			podCIDR, serviceCIDr := coordinatormanager.ExtractK8sCIDRFromKubeadmConfigMap(cm)
+			podCIDR, serviceCIDr, _ := coordinatormanager.ExtractK8sCIDRFromKubeadmConfigMap(cm)


返回的 err 应该被判断一下，避免可能出现的 Extract 失败。

在 ginkgo 中，你可以这样写。

podCIDR, serviceCIDr, err := coordinatormanager.ExtractK8sCIDRFromKubeadmConfigMap(cm) Expect(err).NotTo(HaveOccurred(), "Failed to extract k8s CIDR from Kubeadm configMap, error is %v", err)

cyclinder · 2024-12-10T02:42:09Z

新文件需要有 license 头，补一下
单元测试可以用 ginkgo 书写，补下labels

0x0034 · 2024-12-10T03:10:48Z

新文件需要有 license 头，补一下

单元测试可以用 ginkgo 书写，补下labels

已修改

cyclinder

Thanks! The codes look good!

weizhoublue · 2024-12-11T01:57:36Z

good job

weizhoublue · 2024-12-11T03:04:22Z

the unit test failed:

There were failures detected in the following suites:
coordinatormanager ./pkg/coordinatormanager

Test Suite Failed
exit status 1

0x0034 · 2024-12-12T02:25:35Z

the unit test failed:

There were failures detected in the following suites: coordinatormanager ./pkg/coordinatormanager

Test Suite Failed exit status 1

我晚点来处理一下

weizhoublue · 2024-12-23T02:05:22Z

the unit test failed:
There were failures detected in the following suites: coordinatormanager ./pkg/coordinatormanager
Test Suite Failed exit status 1

我晚点来处理一下

are there any updates ? thanks @0x0034

0x0034 · 2024-12-27T02:34:09Z

so sorry, I need to deal with OKR recently, I got it done this week

cyclinder · 2024-12-27T02:42:03Z

Hi @0x0034, There is a small error. I'm sure it won't take too long to fix it. Today, we are planning a new release that will hopefully include this fix. Do you have time to help out with updating the PR? Or can you open your repository permissions to me so that I can change them for you?

 [FAILED] [0.001 seconds]
Coordinator Manager [BeforeEach] [unittest, informer_test]
/home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:22
  should extract CIDRs correctly
  /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:79
    ClusterConfiguration In One line
    /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:94

  Timeline >>
  > Enter [BeforeEach] Coordinator Manager - /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:22 @ 12/11/24 02:01:22.716
  [FAILED] Failed to unmarshal clusterConfigurationJson
  Unexpected error:
      <*json.SyntaxError | 0xc00041e270>: 
      invalid character '}' looking for beginning of object key string
      {
          msg: "invalid character '}' looking for beginning of object key string",
          Offset: 2318,
      }
  occurred
  In [BeforeEach] at: /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:68 @ 12/11/24 02:01:22.716
  < Exit [BeforeEach] Coordinator Manager - /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:22 @ 12/11/24 02:01:22.716 (1ms)
  << Timeline
------------------------------
• [FAILED] [0.000 seconds]
Coordinator Manager [BeforeEach] [unittest, informer_test]
/home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:22
  should extract CIDRs correctly
  /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:79
    ClusterConfiguration
    /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:101

  Timeline >>
  > Enter [BeforeEach] Coordinator Manager - /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:22 @ 12/11/24 02:01:22.717
  [FAILED] Failed to unmarshal clusterConfigurationJson
  Unexpected error:
      <*json.SyntaxError | 0xc0000201c8>: 
      invalid character '}' looking for beginning of object key string
      {
          msg: "invalid character '}' looking for beginning of object key string",
          Offset: 2318,
      }
  occurred
  In [BeforeEach] at: /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:68 @ 12/11/24 02:01:22.717
  < Exit [BeforeEach] Coordinator Manager - /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:22 @ 12/11/24 02:01:22.717 (0s)
  << Timeline

cyclinder · 2024-12-27T05:25:08Z

coverage: 63.2% of statements
Running Suite: CoordinatorManager Suite - /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager
========================================================================================================
Random Seed: 1735275408 - will randomize all specs

Will run 4 of 4 specs
Running in parallel across 4 processes

Ginkgo timed out waiting for all parallel procs to report back
Test suite: coordinatormanager (./pkg/coordinatormanager)

This occurs if a parallel process exits before it reports its results to the
Ginkgo CLI.  The CLI will now print out all the stdout/stderr output it's
collected from the running processes.  However you may not see anything useful
in these logs because the individual test processes usually intercept output to
stdout/stderr in order to capture it in the spec reports.

You may want to try rerunning your test suite with
--output-interceptor-mode=none to see additional output here and debug your
suite.
  
Output from proc 1:
  Ginkgo detected an issue with your spec structure
  It(testName, func() {
  /home/runner/work/spiderpool/spiderpool/pkg/coordinatormanager/coordinator_informer_test.go:81
    It looks like you are trying to add a [It] node
    to the Ginkgo spec tree in a leaf node after the specs started running.

    To enable randomization and parallelization Ginkgo requires the spec tree
    to be fully constructed up front.  In practice, this means that you can
    only create nodes like [It] at the top-level or within the
    body of a Describe, Context, or When.

    Learn more at:
    http://onsi.github.io/ginkgo/#mental-model-how-ginkgo-traverses-the-spec-hierarchy

  warning: GOCOVERDIR not set, no coverage data emitted

Output from proc 2:
  PASS
  coverage: 0.0% of statements

cyclinder · 2024-12-27T05:30:32Z

确保 It 节点的定义位置正确：
确保所有的 It 节点都在 Describe 或 Context 块的主体内，而不是在其他函数或条件语句中定义。
检查 DescribeTable 的使用：
您的 DescribeTable 和 Entry 应该在 Describe 块的内部，确保没有在测试运行开始后动态添加 It 节点。
避免在 BeforeEach 或其他钩子中定义 It：
确保您没有在 BeforeEach 或其他钩子中定义 It 节点。所有测试应该在 Describe 块中定义。

Signed-off-by: ruochen <[email protected]> Signed-off-by: Cyclinder Kuo <[email protected]>

fix: fail to get podCIDR and serviceCIDR Signed-off-by: robot <[email protected]>

0x0034 requested review from weizhoublue, cyclinder, ty-dc and lou-lan as code owners December 8, 2024 11:16

0x0034 mentioned this pull request Dec 8, 2024

[BUG] serviceCIDR get failed #4307

Closed

ty-dc reviewed Dec 9, 2024

View reviewed changes

cyclinder reviewed Dec 9, 2024

View reviewed changes

0x0034 force-pushed the fix/useClusterConfigurationGetCIDR branch from 218a9d6 to fb36fbe Compare December 9, 2024 03:50

0x0034 force-pushed the fix/useClusterConfigurationGetCIDR branch from fb36fbe to fc5fc86 Compare December 9, 2024 07:18

cyclinder reviewed Dec 9, 2024

View reviewed changes

0x0034 force-pushed the fix/useClusterConfigurationGetCIDR branch from fc5fc86 to b36a7c1 Compare December 9, 2024 09:01

ty-dc reviewed Dec 9, 2024

View reviewed changes

0x0034 force-pushed the fix/useClusterConfigurationGetCIDR branch from b36a7c1 to e22466c Compare December 9, 2024 09:21

cyclinder added the release/bug label Dec 10, 2024

0x0034 force-pushed the fix/useClusterConfigurationGetCIDR branch 2 times, most recently from a2f578f to f454073 Compare December 10, 2024 03:08

cyclinder linked an issue Dec 10, 2024 that may be closed by this pull request

[BUG] serviceCIDR get failed #4307

Closed

cyclinder previously approved these changes Dec 10, 2024

View reviewed changes

weizhoublue changed the title ~~fix: fix get null podCIDR and serviceCIDR~~ fix: fail to get podCIDR and serviceCIDR Dec 11, 2024

weizhoublue added cherrypick-release-v0.8 Cherry-pick the PR to branch release-v0.8. cherrypick-release-v0.9 cherrypick-release-v1.0 Cherry-pick the PR to branch release-v1.0. labels Dec 11, 2024

0x0034 dismissed cyclinder’s stale review via fdce08e December 27, 2024 03:19

0x0034 force-pushed the fix/useClusterConfigurationGetCIDR branch 2 times, most recently from fdce08e to 84b8283 Compare December 27, 2024 03:46

cyclinder force-pushed the fix/useClusterConfigurationGetCIDR branch from 84b8283 to 0bf425a Compare December 27, 2024 06:42

fix: fix get null podCIDR and serviceCIDR

5a5a2c1

Signed-off-by: ruochen <[email protected]> Signed-off-by: Cyclinder Kuo <[email protected]>

cyclinder force-pushed the fix/useClusterConfigurationGetCIDR branch from 0bf425a to 5a5a2c1 Compare December 27, 2024 08:33

cyclinder approved these changes Dec 27, 2024

View reviewed changes

weizhoublue approved these changes Dec 30, 2024

View reviewed changes

weizhoublue merged commit 6587b38 into spidernet-io:main Dec 30, 2024
51 of 54 checks passed

This was referenced Dec 30, 2024

failed to cherry pick PR 4366 from 0x0034, to branch release-v0.8 #4464

Closed

failed to cherry pick PR 4366 from 0x0034, to branch release-v0.9 #4465

Closed

github-actions bot pushed a commit that referenced this pull request Dec 30, 2024

Merge pull request #4366 from 0x0034/fix/useClusterConfigurationGetCIDR

36d780f

fix: fail to get podCIDR and serviceCIDR Signed-off-by: robot <[email protected]>

weizhoublue mentioned this pull request Dec 30, 2024

fix: fail to get podCIDR and serviceCIDR #4466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fail to get podCIDR and serviceCIDR #4366

fix: fail to get podCIDR and serviceCIDR #4366

0x0034 commented Dec 8, 2024 •

edited

Loading

0x0034 commented Dec 9, 2024

ty-dc Dec 9, 2024

cyclinder left a comment

cyclinder Dec 9, 2024

cyclinder commented Dec 9, 2024

cyclinder commented Dec 9, 2024

codecov bot commented Dec 9, 2024 •

edited

Loading

0x0034 commented Dec 9, 2024

ty-dc commented Dec 9, 2024

0x0034 commented Dec 9, 2024 •

edited

Loading

cyclinder Dec 9, 2024

cyclinder Dec 9, 2024

ty-dc Dec 9, 2024

cyclinder commented Dec 10, 2024

0x0034 commented Dec 10, 2024

cyclinder left a comment

weizhoublue commented Dec 11, 2024

weizhoublue commented Dec 11, 2024

0x0034 commented Dec 12, 2024

weizhoublue commented Dec 23, 2024

0x0034 commented Dec 27, 2024

cyclinder commented Dec 27, 2024

cyclinder commented Dec 27, 2024

cyclinder commented Dec 27, 2024

fix: fail to get podCIDR and serviceCIDR #4366

fix: fail to get podCIDR and serviceCIDR #4366

Conversation

0x0034 commented Dec 8, 2024 • edited Loading

Thanks for contributing!

0x0034 commented Dec 9, 2024

ty-dc Dec 9, 2024

Choose a reason for hiding this comment

cyclinder left a comment

Choose a reason for hiding this comment

cyclinder Dec 9, 2024

Choose a reason for hiding this comment

cyclinder commented Dec 9, 2024

cyclinder commented Dec 9, 2024

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

0x0034 commented Dec 9, 2024

ty-dc commented Dec 9, 2024

0x0034 commented Dec 9, 2024 • edited Loading

cyclinder Dec 9, 2024

Choose a reason for hiding this comment

cyclinder Dec 9, 2024

Choose a reason for hiding this comment

ty-dc Dec 9, 2024

Choose a reason for hiding this comment

cyclinder commented Dec 10, 2024

0x0034 commented Dec 10, 2024

cyclinder left a comment

Choose a reason for hiding this comment

weizhoublue commented Dec 11, 2024

weizhoublue commented Dec 11, 2024

0x0034 commented Dec 12, 2024

weizhoublue commented Dec 23, 2024

0x0034 commented Dec 27, 2024

cyclinder commented Dec 27, 2024

cyclinder commented Dec 27, 2024

cyclinder commented Dec 27, 2024

0x0034 commented Dec 8, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

0x0034 commented Dec 9, 2024 •

edited

Loading