fix hash name bug (#479)

go.mod

@@ -23,6 +23,7 @@ require (
 	google.golang.org/grpc v1.65.0
 	google.golang.org/protobuf v1.34.2
 	gopkg.in/natefinch/lumberjack.v2 v2.2.1
+	gopkg.in/yaml.v3 v3.0.1
 	gotest.tools v2.2.0+incompatible
 	istio.io/api v1.22.1
 	istio.io/istio v0.0.0-20240618015532-5764a24ec23c
@@ -222,7 +223,6 @@ require (
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
-	gopkg.in/yaml.v3 v3.0.1 // indirect
 	helm.sh/helm/v3 v3.15.1 // indirect
 	istio.io/client-go v1.22.0-alpha.1.0.20240612141229-fd83cdce6c7d // indirect
 	k8s.io/apiextensions-apiserver v0.30.1 // indirect

pkg/controller/workload/workload_hash.go

@@ -17,23 +17,79 @@
 package workload
 
 import (
+	"fmt"
 	"hash/fnv"
 	"math"
+	"os"
+
+	"gopkg.in/yaml.v3"
 )
 
 var (
 	hash = fnv.New32a()
 )
 
+const (
+	persistPath = "/mnt/workload_hash_name.yaml"
+)
+
 // HashName converts a string to a uint32 integer as the key of bpf map
 type HashName struct {
 	numToStr map[uint32]string
+	strToNum map[string]uint32
 }
 
 func NewHashName() *HashName {
-	con := &HashName{}
-	con.numToStr = make(map[uint32]string)
-	return con
+	hashName := &HashName{
+		strToNum: make(map[string]uint32),
+	}
+	// if read failed, initialize with an empty map
+	if err := hashName.readFromPersistFile(); err != nil {
+		log.Errorf("error reading persist file: %v", err)
+		hashName.numToStr = make(map[uint32]string)
+	} else {
+		hashName.numToStr = make(map[uint32]string, len(hashName.strToNum))
+		for str, num := range hashName.strToNum {
+			hashName.numToStr[num] = str
+		}
+	}
+	return hashName
+}
+
+func (h *HashName) readFromPersistFile() error {
+	data, err := os.ReadFile(persistPath)
+	if err != nil {
+		return err
+	}
+
+	return yaml.Unmarshal(data, &h.strToNum)
+}
+
+func (h *HashName) flush() error {
+	// We only need to flush strToNum here, since we can generate numToStr from it.
+	if len(h.strToNum) == 0 {
+		return os.WriteFile(persistPath, nil, 0644)
+	}
+
+	yaml, err := yaml.Marshal(h.strToNum)
+	if err != nil {
+		return err
+	}
+
+	return os.WriteFile(persistPath, yaml, 0644)
+}
+
+// flushDelta is similar to flush, but it appends new item instead of flush all data
+func (h *HashName) flushDelta(str string, num uint32) error {
+	f, err := os.OpenFile(persistPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+
+	data := fmt.Sprintf("%s: %d\n", str, num)
+	_, err = f.WriteString(data)
+	return err
 }
 
 func (h *HashName) StrToNum(str string) uint32 {
@@ -42,14 +98,26 @@ func (h *HashName) StrToNum(str string) uint32 {
 	hash.Reset()
 	hash.Write([]byte(str))
 
+	if num, exists := h.strToNum[str]; exists {
+		return num
+	}
+
 	// Using linear probing to solve hash conflicts
 	for num = hash.Sum32(); num < math.MaxUint32; num++ {
-		if h.numToStr[num] == "" {
+		// Create a new item if we find an empty slot
+		if _, exists := h.numToStr[num]; !exists {
 			h.numToStr[num] = str
-			break
-		} else if h.numToStr[num] == str {
+			h.strToNum[str] = num
+			// Create a new item here, should flush
+			if err := h.flushDelta(str, num); err != nil {
+				log.Errorf("error flushing when calling StrToNum: %v", err)
+			}
 			break
 		}
+		// It's a ring
+		if num == math.MaxUint32 {
+			num = 0
+		}
 	}
 
 	return num
@@ -60,5 +128,13 @@ func (h *HashName) NumToStr(num uint32) string {
 }
 
 func (h *HashName) Delete(str string) {
-	delete(h.numToStr, h.StrToNum(str))
+	// only when the num exists, we do the logic
+	if num, exists := h.strToNum[str]; exists {
+		delete(h.numToStr, num)
+		delete(h.strToNum, str)
+		// delete an old item here, should flush
+		if err := h.flush(); err != nil {
+			log.Errorf("error flushing when calling Delete: %v", err)
+		}
+	}
 }

pkg/controller/workload/workload_hash_test.go

@@ -0,0 +1,113 @@
+/*
+ * Copyright 2024 The Kmesh Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package workload
+
+import (
+	"hash/fnv"
+	"os"
+	"testing"
+)
+
+func getHashValueMap(testStrings []string) map[string]uint32 {
+	hashValueMap := make(map[string]uint32)
+	hash := fnv.New32a()
+	for _, testString := range testStrings {
+		hash.Reset()
+		hash.Write([]byte(testString))
+		hashValueMap[testString] = hash.Sum32()
+	}
+	return hashValueMap
+}
+
+// clean persist file for test
+func cleanPersistFile() {
+	_ = os.Remove(persistPath)
+}
+
+func TestWorkloadHash_Basic(t *testing.T) {
+	cleanPersistFile()
+	hashName := NewHashName()
+
+	// "foo" does not collide with "bar"
+	// while "costarring" collides with "liquid"
+	testStrings := []string{
+		"foo", "bar", "costarring", "liquid",
+	}
+	hashValueMap := getHashValueMap(testStrings)
+
+	testcases := []struct {
+		str         string
+		expectedNum uint32
+	}{
+		{"foo", hashValueMap["foo"]},
+		{"bar", hashValueMap["bar"]},
+		{"costarring", hashValueMap["costarring"]},
+		// collision occurs here, so plus 1
+		{"liquid", hashValueMap["costarring"] + 1},
+	}
+
+	for _, testcase := range testcases {
+		str := testcase.str
+		expectedNum := testcase.expectedNum
+
+		actualNum := hashName.StrToNum(str)
+		if actualNum != expectedNum {
+			t.Errorf("StrToNum(%s) = %d, want %d", str, actualNum, expectedNum)
+		}
+
+		// Test Number to String
+		actualStr := hashName.NumToStr(actualNum)
+		if actualStr != str {
+			t.Errorf("NumToStr(%d) = %s, want %s", actualNum, actualStr, str)
+		}
+	}
+}
+
+func TestWorkloadHash_StrToNumAfterDelete(t *testing.T) {
+	cleanPersistFile()
+	testStrings := []string{
+		"foo", "bar", "costarring", "liquid",
+	}
+	strToNumMap := make(map[string]uint32)
+	hashName := NewHashName()
+	for _, testString := range testStrings {
+		num := hashName.StrToNum(testString)
+		strToNumMap[testString] = num
+	}
+
+	// create a new one to imutate the kmesh restart
+	hashName = NewHashName()
+	// we swap the two collided strings
+	testStrings[2], testStrings[3] = testStrings[3], testStrings[2]
+	for _, testString := range testStrings {
+		actualNum := hashName.StrToNum(testString)
+		expectedNum := strToNumMap[testString]
+		if actualNum != expectedNum {
+			t.Errorf("StrToNum(%s) = %d, want %d", testString, actualNum, expectedNum)
+		}
+	}
+
+	for _, testString := range testStrings {
+		hashName.Delete(testString)
+		originalNum := strToNumMap[testString]
+		gotString := hashName.NumToStr(originalNum)
+		if gotString != "" {
+			t.Errorf("String of number %d should be empty, but got %s", originalNum, gotString)
+			return
+		}
+	}
+}