Sort dump output by name #1432

zrggw · 2025-06-27T10:05:00Z

What type of PR is this?
/kind enhancement

What this PR does / why we need it:
#1431
Because the output of "kmeshctl dump" is unordered, it is inconvenient to compare the results from two different uses of dump. So I sorted it by "name".

Does this PR introduce a user-facing change?:

kmesh-bot · 2025-06-27T10:05:12Z

Welcome @zrggw! It looks like this is your first PR to kmesh-net/kmesh 🎉

hzxuzhonghu · 2025-06-28T08:22:42Z

LGTM

Can you add test cases to the existing test

hzxuzhonghu · 2025-07-02T02:59:18Z

why do you change the bpf generated file for kernel native mode

hzxuzhonghu · 2025-07-02T02:59:37Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces sorting to the kmeshctl dump output, which is a valuable enhancement for ensuring deterministic output for comparisons and testing. The implementation in pkg/status/status_server.go is correct and achieves the desired outcome.

The tests have been significantly expanded to validate this new behavior. However, I've identified a couple of areas for improvement in pkg/status/status_server_test.go. Specifically, there's some ineffective code related to shuffling test data, and the test unconditionally writes golden files, which is not ideal. My comments provide suggestions to address these points.

gemini-code-assist · 2025-07-02T03:00:48Z

pkg/status/status_server_test.go

+	rand.New(rand.NewSource(time.Now().UnixNano()))
+	rand.Shuffle(len(workloads), func(i, j int) {
+		workloads[i], workloads[j] = workloads[j], workloads[i]
+	})
+	rand.Shuffle(len(services), func(i, j int) {
+		services[i], services[j] = services[j], services[i]
+	})


The rand.Shuffle calls on the workloads and services slices have no effect on the test outcome. The server.configDumpWorkload function retrieves data from the cache within the server object, which was populated before this shuffle. The local workloads and services slices are not used after being shuffled.

The non-deterministic nature of map iteration within the cache's List() method is sufficient to test that the output is correctly sorted.

Additionally, rand.New(...) creates a new rand.Rand object that is not used, so this line is a no-op. Please remove these lines.

gemini-code-assist · 2025-07-02T03:00:48Z

pkg/status/status_server_test.go

+	// save original and modified dumps to json files
+	err = os.WriteFile("./testdata/workload_configdump_original.json", w2.Body.Bytes(), 0644)
+	if err != nil {
+		t.Errorf("Failed to write original dump to file: %v", err)
+	}
+
+	err = os.WriteFile("./testdata/workload_configdump_modified.json", w3.Body.Bytes(), 0644)
+	if err != nil {
+		t.Errorf("Failed to write modified dump to file: %v", err)
+	}


This test unconditionally writes to workload_configdump_original.json and workload_configdump_modified.json. Tests should not write files to the source tree during normal execution as it can cause side effects and make tests non-hermetic.

If the goal is to update golden files, this should be controlled by a flag (e.g., an -update flag). Since these files are already part of the pull request, this logic seems unnecessary for the test itself. Consider removing these os.WriteFile calls. The existing util.RefreshGoldenFile is a better pattern for managing golden files.

Copilot

Pull Request Overview

This PR ensures the output of kmeshctl dump is consistently ordered by name and adds tests to verify this behavior.

Added sorting of workloads, services, and policies in the server before marshalling JSON.
Refactored and expanded the integration test (TestServer_configDumpWorkload) with helpers, multiple scenarios, shuffle-based verification, and golden file generation.
Added two JSON fixtures (workload_configdump_original.json and workload_configdump_modified.json) for the new test.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

File	Description
pkg/status/status_server.go	Added `sort.Slice` calls to order workloads, services, and policies
pkg/status/status_server_test.go	Rewrote dump test with `buildWorkload`/`buildService`, multiple dumps, shuffling, and golden-file logic
pkg/status/testdata/workload_configdump_original.json	New fixture representing the original sorted dump
pkg/status/testdata/workload_configdump_modified.json	New fixture representing the modified sorted dump

Copilot · 2025-07-02T03:05:34Z

pkg/status/status_server_test.go

+	rand.New(rand.NewSource(time.Now().UnixNano()))
+	rand.Shuffle(len(workloads), func(i, j int) {
+		workloads[i], workloads[j] = workloads[j], workloads[i]
+	})
+	rand.Shuffle(len(services), func(i, j int) {


Calling rand.New without using its returned *Rand makes the seeding ineffective; use rand.Seed to seed the global RNG or assign the new Rand instance to a variable and use it for Shuffle.

Suggested change

rand.New(rand.NewSource(time.Now().UnixNano()))

rand.Shuffle(len(workloads), func(i, j int) {

workloads[i], workloads[j] = workloads[j], workloads[i]

})

rand.Shuffle(len(services), func(i, j int) {

r := rand.New(rand.NewSource(time.Now().UnixNano()))

r.Shuffle(len(workloads), func(i, j int) {

workloads[i], workloads[j] = workloads[j], workloads[i]

})

r.Shuffle(len(services), func(i, j int) {

Copilot · 2025-07-02T03:05:34Z

pkg/status/status_server_test.go

+	rand.New(rand.NewSource(time.Now().UnixNano()))
+	rand.Shuffle(len(workloads), func(i, j int) {
+		workloads[i], workloads[j] = workloads[j], workloads[i]
+	})
+	rand.Shuffle(len(services), func(i, j int) {
+		services[i], services[j] = services[j], services[i]
+	})
+


Shuffling the workloads slice after populating the cache has no effect on the cache’s internal order; move the Shuffle call to before adding entries to fakeWorkloadCache so the test actually verifies sorting.

Suggested change

rand.New(rand.NewSource(time.Now().UnixNano()))

rand.Shuffle(len(workloads), func(i, j int) {

workloads[i], workloads[j] = workloads[j], workloads[i]

})

rand.Shuffle(len(services), func(i, j int) {

services[i], services[j] = services[j], services[i]

})

pkg/status/status_server_test.go

Copilot · 2025-07-02T03:05:35Z

pkg/status/status_server_test.go

+	// compare the modified dump with the original
+	err := util.Compare(w3.Body.Bytes(), w2.Body.Bytes())
+	if err != nil {
+		fmt.Printf("Modified dump differs from original: %v\n", err)


Use t.Logf instead of fmt.Printf in tests so log output is captured by the test runner; consider replacing this call with t.Logf.

Suggested change

fmt.Printf("Modified dump differs from original: %v\n", err)

t.Logf("Modified dump differs from original: %v", err)

hzxuzhonghu · 2025-07-02T03:14:35Z

pkg/status/status_server_test.go


-	util.RefreshGoldenFile(t, w.Body.Bytes(), "./testdata/workload_configdump.json")
+	// Create a new HTTP request and response
+	req2 := httptest.NewRequest(http.MethodGet, "/configDumpWorkload", nil)


I feel we donot need to send a second req, mixing the workloads and services at first, we can test getting only once.

zrggw · 2025-07-02T03:14:44Z

why do you change the bpf generated file for kernel native mode

The generated files under the kernelnative root directory were created incorrectly, and I will delete these files later.

hzxuzhonghu · 2025-07-02T03:26:17Z

The generated files under the kernelnative root directory were created incorrectly, and I will delete these files later.

@lec-bit Can you confirm?

zrggw · 2025-07-02T09:10:28Z

I revise the code according to the code review results.

remove second and third req.
remove rand.
remove generated files under the kernelnative root directory.

codecov · 2025-07-02T09:27:57Z

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 35.86%. Comparing base (8f619b9) to head (81896f5).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/status/status_server.go	66.66%	2 Missing and 1 partial ⚠️

Files with missing lines	Coverage Δ
pkg/status/status_server.go	`34.04% <66.66%> (+0.70%)`	⬆️

... and 32 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update accb01d...81896f5. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

LiZhenCheng9527 · 2025-07-03T06:24:08Z

config/kmesh_marcos_def.h

 * loopback address link conflicts. Obtains the namespace cookie of the
 * current container based on the bpf_get_netns_cookie auxiliary function.
 */
-#define MDA_LOOPBACK_ADDR 1


use make clean command clean up if you have build kmesh code locally before pushing your code to github.

Signed-off-by: aicee <hhbin2000@foxmail.com>

…cording to the code review results. Signed-off-by: aicee <hhbin2000@foxmail.com>

hzxuzhonghu · 2025-07-04T02:08:25Z

pkg/status/status_server_test.go

+		// Modify service properties
+		svc := buildService(fmt.Sprintf("service-%d-modified", i), fmt.Sprintf("hostname-%d-modified", i))
+		// Modify service ports
+		svc.Ports = []*workloadapi.Port{


why modify ports, does it influence the order?

Just to test that the sorting is not affected by other modifications, like port modifications.

hzxuzhonghu · 2025-07-04T02:09:29Z

pkg/status/status_server_test.go

+	services := []*workloadapi.Service{}
+
+	for i := 0; i < 10; i++ {
+		w := buildWorkload(fmt.Sprintf("workload-%d", i))


Isn't this make the workloads alreadt sorted?

Later, when w is added to fakeWorkloadCache(line 326) and then a workDump is created(line 346 and status_servers.go, line 493), the wokload in workloadDump will be unordered due to the use of map.

hzxuzhonghu · 2025-07-10T01:35:18Z

please fix the ci

Signed-off-by: aicee <hhbin2000@foxmail.com>

zrggw · 2025-07-10T02:38:16Z

/retest

kmesh-bot · 2025-07-10T02:38:38Z

@zrggw: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hzxuzhonghu · 2025-07-10T02:54:32Z

/ok-to-test

/retest

zrggw · 2025-07-10T03:11:55Z

/retest

hzxuzhonghu · 2025-07-10T03:42:35Z

Looks this failure is related

Signed-off-by: aicee <hhbin2000@foxmail.com>

hzxuzhonghu · 2025-07-15T06:51:46Z

pkg/controller/security/manager_test.go

+	secretManager.SendCertRequest(identity, RETRY)
 	time.Sleep(2000 * time.Millisecond)
-	assert.NotNil(t, secretManager.GetCert(identity).cert)
+	for {


nit: please use retry.UntilSuccess, which also have a timeout

And seems we can remove L168 now

It seems we can also remove line 182-186? Because we have already checked whether cert.cert is nil on line 173.

Signed-off-by: aicee <hhbin2000@foxmail.com>

LiZhenCheng9527 · 2025-07-15T07:57:01Z

/lgtm

hzxuzhonghu

/approve

kmesh-bot · 2025-07-17T07:08:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controller/OWNERS~~ [hzxuzhonghu]
~~pkg/status/OWNERS~~ [hzxuzhonghu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot AI review requested due to automatic review settings June 27, 2025 10:05

kmesh-bot added the kind/enhancement New feature or request label Jun 27, 2025

kmesh-bot requested review from Okabe-Rintarou-0 and hzxuzhonghu June 27, 2025 10:05

kmesh-bot added the size/S label Jun 27, 2025

This comment was marked as outdated.

Sign in to view

kmesh-bot added size/XXL and removed size/S labels Jul 2, 2025

gemini-code-assist bot reviewed Jul 2, 2025

View reviewed changes

hzxuzhonghu requested a review from Copilot July 2, 2025 03:03

Copilot AI reviewed Jul 2, 2025

View reviewed changes

hzxuzhonghu reviewed Jul 2, 2025

View reviewed changes

zrggw force-pushed the sort_dump_output branch from ab62d27 to 5b73a5f Compare July 2, 2025 09:06

LiZhenCheng9527 reviewed Jul 3, 2025

View reviewed changes

zrggw force-pushed the sort_dump_output branch from 5b73a5f to b8d76f0 Compare July 3, 2025 06:45

zrggw added 3 commits July 3, 2025 14:49

Sort dump output by name

cac60ce

Signed-off-by: aicee <hhbin2000@foxmail.com>

add unit test for sorting dump output by name

6b54579

Signed-off-by: aicee <hhbin2000@foxmail.com>

add unit test for sorting dump output by name, and revise the code ac…

52bc681

…cording to the code review results. Signed-off-by: aicee <hhbin2000@foxmail.com>

zrggw force-pushed the sort_dump_output branch from b8d76f0 to 52bc681 Compare July 3, 2025 06:54

hzxuzhonghu reviewed Jul 4, 2025

View reviewed changes

hzxuzhonghu previously approved these changes Jul 4, 2025

View reviewed changes

kmesh-bot added the approved label Jul 4, 2025

fix: nil pointer dereference in runTestCertRotate function.

0690f18

Signed-off-by: aicee <hhbin2000@foxmail.com>

zrggw dismissed hzxuzhonghu’s stale review via 0690f18 July 10, 2025 01:43

kmesh-bot removed the approved label Jul 10, 2025

zrggw mentioned this pull request Jul 10, 2025

Flaky Test: TestSecurity, runTestretryFetchCert #1437

Closed

kmesh-bot added the ok-to-test label Jul 10, 2025

try to fix runTestretryFetchCert nil error

04935ed

Signed-off-by: aicee <hhbin2000@foxmail.com>

hzxuzhonghu reviewed Jul 15, 2025

View reviewed changes

use retry.UntilSucess

81896f5

Signed-off-by: aicee <hhbin2000@foxmail.com>

kmesh-bot assigned LiZhenCheng9527 Jul 15, 2025

kmesh-bot added the lgtm label Jul 15, 2025

hzxuzhonghu reviewed Jul 17, 2025

View reviewed changes

kmesh-bot added the approved label Jul 17, 2025

kmesh-bot merged commit e35895c into kmesh-net:main Jul 17, 2025
12 checks passed

zrggw mentioned this pull request Aug 26, 2025

REQUEST: New membership for Haobin Huang #1492

Closed

6 tasks

	fmt.Printf("Modified dump differs from original: %v\n", err)
	t.Logf("Modified dump differs from original: %v", err)

Sort dump output by name #1432

Sort dump output by name #1432

Uh oh!

Conversation

zrggw commented Jun 27, 2025

Uh oh!

kmesh-bot commented Jun 27, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

hzxuzhonghu commented Jun 28, 2025

Uh oh!

hzxuzhonghu commented Jul 2, 2025

Uh oh!

hzxuzhonghu commented Jul 2, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zrggw commented Jul 2, 2025

Uh oh!

hzxuzhonghu commented Jul 2, 2025

Uh oh!

zrggw commented Jul 2, 2025

Uh oh!

codecov bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zrggw Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented Jul 10, 2025

Uh oh!

zrggw commented Jul 10, 2025

Uh oh!

kmesh-bot commented Jul 10, 2025

Uh oh!

hzxuzhonghu commented Jul 10, 2025

Uh oh!

zrggw commented Jul 10, 2025

Uh oh!

hzxuzhonghu commented Jul 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Jul 2, 2025 •

edited

Loading

zrggw Jul 4, 2025 •

edited

Loading