fix(cache): assign real cluster ids in kernel-native mode

[opx0]

What type of PR is this?

/kind bug

What this PR does / why we need it:

In kernel-native mode every cluster is flushed to the BPF map with Id = 0.

ClusterCache.Flush took the id from HashName.StrToNum, which is a plain map read rather than the allocator:

func (h *HashName) StrToNum(str string) uint32 {
	return h.strToNum[str]
}

HashName.Hash is the only function that inserts into strToNum, and it is called exclusively from pkg/controller/workload/workload_processor.go — the workload path. Nothing on the ads path ever calls it. NewAdsCache builds a fresh utils.NewHashName() and hands it only to NewClusterCache, and the persist file that NewHashName restores from is written only by Hash and Delete, so in kernel-native mode it stays empty too. The lookup therefore returned the zero value for every cluster.

That matters because circuit breaker stats are keyed by {netns_cookie, cluster_id} (bpf/kmesh/ads/include/circuit_breaker.h), and on_cluster_sock_bind reads the id straight off the cluster:

struct cluster_stats_key {
    __u64 netns_cookie;
    __u32 cluster_id;
};

So with every id at 0:

• All clusters in a network namespace collapse onto a single cluster_stats entry. A pod calling reviews (maxConnections: 10) and ratings (maxConnections: 100) shares one counter: after 10 combined connections on_cluster_sock_bind returns -1 and further connections to reviews are rejected, while ratings is throttled at the wrong threshold.
• clearClusterStats resolves clusterId = 0 and BatchDeletes every key with ClusterId == 0, which is all of them. Removing one cluster wipes the live connection counters of every cluster in every netns, and subsequent closes then hit the -delta > stats->active_connections guard, so the counters stay skewed.

The fix is to use the allocator:

cluster.Id = cache.hashName.Hash(name)

Hash returns the existing id when the name is already registered, so a cluster keeps its id across config pushes and its stats entries stay reachable.

The second commit is a test-only fix. TestClearClusterStats built its stats keys from StrToNum on a HashName nothing had registered into, so all five keys carried ClusterId 0 and collapsed to three distinct entries; the first clearClusterStats call then deleted all of them and the per-cluster assertions never ran. It now registers the names through Hash and asserts the whole remaining map, which is the property the test is named for: clearing one cluster leaves the other clusters' counters untouched.

Which issue(s) this PR fixes:
Fixes #1936

Special notes for your reviewer:

• clearClusterStats deliberately stays on StrToNum. It resolves an id that Flush has already registered, and it runs before hashName.Delete(name) on the delete path, so the entry is still present. Using the allocator there would mint an id for a cluster that is being removed.
• New behaviour worth knowing: because Hash persists new entries, the ads path now writes /mnt/hash_name.yaml, which it never did before. /mnt is already a hostPath mount in the daemonset (deploy/yaml/kmesh.yaml, deploy/charts/kmesh-helm/templates/daemonset.yaml), and this is what makes cluster ids survive a restart, so the cluster_stats entries written before the restart stay addressable afterwards.
• Relation to HashName.Hash never reaches the final ring slot (math.MaxUint32); wraparound is dead code #1904. This makes the ads path depend on HashName.Hash, which has a separate edge case: for num = h.hash.Sum32(); num < math.MaxUint32; num++ never enters the body when the hash is exactly math.MaxUint32, so it returns an id it never registered. On this path that would mean a live cluster whose name is missing from strToNum, and a later clearClusterStats resolving 0 for it. It is a 1-in-2^32 case and fix(utils): make HashName ring probe reach the final slot #1905 already fixes it, so I have not touched it here; flagging the interaction since it becomes reachable from the ads path with this change.
• Verification. TestClusterFlushAssignsIds fails on the unmodified line (Should not be zero, but was 0 for both clusters, and the two ids equal) and passes with the fix, under the same -race -gcflags=all=-l flags CI uses. pkg/controller/ads/... and the rest of pkg/cache/v2 show no new failures against main; five tests in pkg/cache/v2 (TestClusterLookupAll, TestClearClusterStats, TestListenerLookupAll, TestListenerFlushAndLookup, TestListenerUpdateAndDeleteFlush) fail identically on unmodified main in my environment because I cannot attach real BPF maps there. That means the TestClearClusterStats change in the second commit is exercised by CI rather than by me locally.
• Coverage I did not add. @AnouarMohamed suggested asserting that two clusters with different circuit-breaker limits do not share cluster_stats. That property lives in the BPF map, not in the Go cache, so it needs an e2e test rather than a unit test; what the unit test can show is what it now shows, that the two clusters resolve to distinct non-zero ids, with the differing maxConnections in the fixture to keep the scenario recognisable. Happy to follow up with the e2e case if you would like it.
• AI disclosure, per CONTRIBUTING.md: I used Claude Code while preparing this PR — tracing the call paths, drafting the tests, and drafting this description. I have reviewed and run every change myself.

Does this PR introduce a user-facing change?:

Fix kernel-native mode assigning cluster id 0 to every cluster, which made all clusters in a network namespace share a single circuit breaker counter and let the removal of one cluster wipe every cluster's connection stats.

[kmesh-bot]

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

[kmesh-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign okabe-rintarou-0 for approval. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• pkg/OWNERS

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

[Copilot]

Pull request overview

Fixes kernel-native mode cluster flushing so clusters receive stable, non-zero IDs (instead of always 0), preventing circuit-breaker stats collisions keyed by {netns_cookie, cluster_id}.

Changes:

• Update ClusterCache.Flush to allocate cluster IDs via HashName.Hash(name) rather than a plain lookup.
• Fix TestClearClusterStats to register cluster names through Hash() and assert the remaining stats map precisely after deletions.
• Add TestClusterFlushAssignsIds to assert cluster IDs are non-zero, distinct across clusters, and stable across multiple flushes.

Reviewed changes

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

File	Description
pkg/cache/v2/cluster.go	Allocate/persist cluster IDs during flush using Hash() so ADS/kernel-native mode no longer emits Id=0 for every cluster.
pkg/cache/v2/cluster_test.go	Update stats-clearing test to use registered IDs and add coverage ensuring flush assigns stable, distinct non-zero IDs.

Suppressed comments (2)

pkg/cache/v2/cluster_test.go:433

• This subtest creates a HashName backed by the global persist file ("/mnt/hash_name.yaml") and deletes it via Reset(). Since CI runs package tests in parallel, this shared file can be mutated concurrently by other packages' tests that also use HashName, leading to flaky results. Prefer injecting a temp persist path per test (t.TempDir) once HashName supports it.

	t.Run("clusters get distinct non-zero ids", func(t *testing.T) {
		hashName := utils.NewHashName()
		defer hashName.Reset()
		cache := NewClusterCache(nil, hashName)

pkg/cache/v2/cluster_test.go:449

• Same as above: this subtest uses utils.NewHashName() (global /mnt/hash_name.yaml) and deletes it via Reset(), which can race with other packages' tests under parallel go test. Once HashName allows configuring the persist path, switch tests to a per-test temp file to avoid cross-package interference.

	t.Run("id is stable across flushes", func(t *testing.T) {
		hashName := utils.NewHashName()
		defer hashName.Reset()
		cache := NewClusterCache(nil, hashName)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.58%. Comparing base (c6357aa) to head (06082ed).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Coverage Δ
pkg/cache/v2/cluster.go	45.31% <100.00%> (ø)

... and 1 file with indirect coverage changes

---

Continue to review full report in Codecov by Harness.

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

🚀 New features to boost your workflow:

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

[Aryan-B2107]

@opx0

I’ve taken the e2e for point 2 (two clusters with different circuit-breaker limits must not share cluster_stats).

The test is TestCircuitBreakerClusterStatsIsolation: one client pod, DestinationRules with maxConnections: 10 and 100 (same reviews/ratings scenario), hold more than 10 connections on the high-limit cluster, then assert the low-limit cluster still accepts. That is the BPF-map property the unit test cannot cover.

I will keep this off #1938 so we don’t both land the Hash/Id fix. After this merges I’ll open a follow-up with that test and ping you for review.

If you’d rather have the e2e on this PR, say so and I can send the commits here instead.

[opx0]

@Aryan-B2107
This is great thanks for taking it on. The setup is exactly right: holding more than 10 connections on the high-limit cluster and then confirming the low-limit one still accepts is precisely the isolation that lives in the BPF map, which is the part a unit test just can't reach. Good to have it nailed down.

Let's do it as a follow-up, the way you suggested. It keeps things clean on both sides — #1938 stays a small, one-line fix with its unit tests, so it's easy to review and merge, and your e2e gets to run against main once the fix is actually there, which is what it needs anyway. Bundling them would just gate the little fix behind the full Istio CI for no real gain.

So I'll focus on getting #1938 in, then ping me the moment your follow-up is up and I'll review it right away. Appreciate you jumping on this one.

[Aryan-B2107]

Sounds good, follow-up after this merges, e2e only.

Could you ping me when #1938 is in? I’ll open the PR against main right after and tag you for review.