From cca6d12be8b0a6fa38aa5f3cbc8b897c496e806f Mon Sep 17 00:00:00 2001 From: fgksgf Date: Fri, 10 Jan 2025 17:40:19 +0800 Subject: [PATCH 1/5] fix that pod spec hash may be changed when new fields are added to pod spec --- pkg/controllers/pd/tasks/pod_test.go | 11 +++++++---- pkg/utils/k8s/pod.go | 15 ++++++++++----- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/pkg/controllers/pd/tasks/pod_test.go b/pkg/controllers/pd/tasks/pod_test.go index eabe13cbec..ed3b3a4279 100644 --- a/pkg/controllers/pd/tasks/pod_test.go +++ b/pkg/controllers/pd/tasks/pod_test.go @@ -35,7 +35,10 @@ import ( "github.com/pingcap/tidb-operator/pkg/utils/task/v3" ) -const fakeVersion = "v1.2.3" +const ( + fakeVersion = "v1.2.3" + expectedPodSpecHash = "765648cb46" +) func TestTaskPod(t *testing.T) { cases := []struct { @@ -283,7 +286,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-pd-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "6d6499ffc7", + v1alpha1.LabelKeyPodSpecHash: expectedPodSpecHash, } return obj }), @@ -332,7 +335,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-pd-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "6d6499ffc7", + v1alpha1.LabelKeyPodSpecHash: expectedPodSpecHash, "xxx": "yyy", } return obj @@ -383,7 +386,7 @@ func TestTaskPod(t *testing.T) { obj.Labels = map[string]string{ v1alpha1.LabelKeyInstance: "aaa-xxx", v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "6d6499ffc7", + v1alpha1.LabelKeyPodSpecHash: expectedPodSpecHash, } return obj }), diff --git a/pkg/utils/k8s/pod.go b/pkg/utils/k8s/pod.go index ed4b25d78e..4f0b665be5 100644 --- a/pkg/utils/k8s/pod.go +++ b/pkg/utils/k8s/pod.go @@ -15,6 +15,7 @@ package k8s import ( + "encoding/json" "fmt" "hash/fnv" @@ -29,17 +30,21 @@ import ( // CalculateHashAndSetLabels calculate the hash of pod spec and set it to the pod labels. func CalculateHashAndSetLabels(pod *corev1.Pod) { - hasher := fnv.New32a() - if pod.Labels == nil { - pod.Labels = map[string]string{} - } spec := pod.Spec.DeepCopy() for i := range spec.InitContainers { c := &spec.InitContainers[i] // ignores init containers image change to support hot reload image for sidecar c.Image = "" } - hashutil.DeepHashObject(hasher, spec) + + // This prevents the hash from being changed when new fields are added to the `PodSpec` due to K8s version upgrades. + data, _ := json.Marshal(spec) + hasher := fnv.New32a() + hashutil.DeepHashObject(hasher, data) + + if pod.Labels == nil { + pod.Labels = map[string]string{} + } pod.Labels[v1alpha1.LabelKeyPodSpecHash] = rand.SafeEncodeString(fmt.Sprint(hasher.Sum32())) } From c1721581930299b8bc63f0dad29e6d1aca6b94da Mon Sep 17 00:00:00 2001 From: fgksgf Date: Mon, 13 Jan 2025 16:28:55 +0800 Subject: [PATCH 2/5] ignore coverage.txt instead of deleting it --- .gitignore | 1 + Makefile | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 2c060ba410..0e35464892 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ _output output .idea/ +coverage.txt diff --git a/Makefile b/Makefile index f7e99b1525..d327e6c5f6 100644 --- a/Makefile +++ b/Makefile @@ -121,7 +121,7 @@ lint: bin/golangci-lint unit: go test $$(go list -e ./... | grep -v cmd | grep -v tools | grep -v tests | grep -v third_party) \ -cover -coverprofile=coverage.txt -covermode=atomic - sed -i.bak '/generated/d;/fake.go/d' coverage.txt && rm coverage.txt coverage.txt.bak + sed -i.bak '/generated/d;/fake.go/d' coverage.txt && rm coverage.txt.bak .PHONY: check check: lint unit verify From 957df6338eef8fb833b4abc9c8e8422c75a871a4 Mon Sep 17 00:00:00 2001 From: fgksgf Date: Mon, 13 Jan 2025 16:35:05 +0800 Subject: [PATCH 3/5] fix UT --- pkg/controllers/tidb/tasks/pod_test.go | 2 +- pkg/controllers/tiflash/tasks/pod_test.go | 15 +++++++++------ pkg/controllers/tikv/tasks/pod_test.go | 15 +++++++++------ 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/pkg/controllers/tidb/tasks/pod_test.go b/pkg/controllers/tidb/tasks/pod_test.go index 74b79759f3..121dbb8dc0 100644 --- a/pkg/controllers/tidb/tasks/pod_test.go +++ b/pkg/controllers/tidb/tasks/pod_test.go @@ -32,7 +32,7 @@ import ( const ( fakeVersion = "v1.2.3" - podSpecHash = "9bbf8897b" + podSpecHash = "6cfdc7d895" ) func TestTaskPod(t *testing.T) { diff --git a/pkg/controllers/tiflash/tasks/pod_test.go b/pkg/controllers/tiflash/tasks/pod_test.go index 5212716c97..259d94c2a4 100644 --- a/pkg/controllers/tiflash/tasks/pod_test.go +++ b/pkg/controllers/tiflash/tasks/pod_test.go @@ -30,7 +30,10 @@ import ( "github.com/pingcap/tidb-operator/pkg/utils/task/v3" ) -const fakeVersion = "v1.2.3" +const ( + fakeVersion = "v1.2.3" + podSpecHash = "5575684b96" +) func TestTaskPod(t *testing.T) { cases := []struct { @@ -123,7 +126,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-tiflash-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "556796549d", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, } return obj }), @@ -147,7 +150,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-tiflash-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "old", - v1alpha1.LabelKeyPodSpecHash: "556796549d", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, } return obj }), @@ -171,7 +174,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-tiflash-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "556796549d", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, "xxx": "yyy", } return obj @@ -196,7 +199,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-tiflash-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "556796549d", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, "xxx": "yyy", } return obj @@ -222,7 +225,7 @@ func TestTaskPod(t *testing.T) { obj.Labels = map[string]string{ v1alpha1.LabelKeyInstance: "aaa-xxx", v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "556796549d", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, } return obj }), diff --git a/pkg/controllers/tikv/tasks/pod_test.go b/pkg/controllers/tikv/tasks/pod_test.go index e6abe27c34..174c4aed2c 100644 --- a/pkg/controllers/tikv/tasks/pod_test.go +++ b/pkg/controllers/tikv/tasks/pod_test.go @@ -33,7 +33,10 @@ import ( "github.com/pingcap/tidb-operator/pkg/utils/task/v3" ) -const fakeVersion = "v1.2.3" +const ( + fakeVersion = "v1.2.3" + podSpecHash = "6b85d6945f" +) func TestTaskPod(t *testing.T) { now := metav1.Now() @@ -173,7 +176,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-tikv-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "59f798884b", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, } return obj }), @@ -197,7 +200,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-tikv-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "old", - v1alpha1.LabelKeyPodSpecHash: "59f798884b", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, } return obj }), @@ -221,7 +224,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-tikv-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "59f798884b", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, "xxx": "yyy", } return obj @@ -246,7 +249,7 @@ func TestTaskPod(t *testing.T) { pod: fake.FakeObj("aaa-tikv-xxx", func(obj *corev1.Pod) *corev1.Pod { obj.Labels = map[string]string{ v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "59f798884b", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, "xxx": "yyy", } return obj @@ -272,7 +275,7 @@ func TestTaskPod(t *testing.T) { obj.Labels = map[string]string{ v1alpha1.LabelKeyInstance: "aaa-xxx", v1alpha1.LabelKeyConfigHash: "newest", - v1alpha1.LabelKeyPodSpecHash: "59f798884b", + v1alpha1.LabelKeyPodSpecHash: podSpecHash, } return obj }), From 604feb04e3054e7579e158771ea56c249f3d673a Mon Sep 17 00:00:00 2001 From: fgksgf Date: Wed, 15 Jan 2025 16:55:06 +0800 Subject: [PATCH 4/5] use k8s native encoder --- pkg/controllers/pd/tasks/pod_test.go | 2 +- pkg/controllers/tidb/tasks/pod_test.go | 2 +- pkg/controllers/tiflash/tasks/pod_test.go | 2 +- pkg/controllers/tikv/tasks/pod_test.go | 2 +- pkg/utils/k8s/pod.go | 22 ++++++++++++++++--- pkg/utils/k8s/revision/controller_revision.go | 1 + 6 files changed, 24 insertions(+), 7 deletions(-) diff --git a/pkg/controllers/pd/tasks/pod_test.go b/pkg/controllers/pd/tasks/pod_test.go index 7efd5e62d6..131de3cc31 100644 --- a/pkg/controllers/pd/tasks/pod_test.go +++ b/pkg/controllers/pd/tasks/pod_test.go @@ -37,7 +37,7 @@ import ( const ( fakeVersion = "v1.2.3" - expectedPodSpecHash = "765648cb46" + expectedPodSpecHash = "7d4f6bf985" ) func TestTaskPod(t *testing.T) { diff --git a/pkg/controllers/tidb/tasks/pod_test.go b/pkg/controllers/tidb/tasks/pod_test.go index 121dbb8dc0..8ee029f487 100644 --- a/pkg/controllers/tidb/tasks/pod_test.go +++ b/pkg/controllers/tidb/tasks/pod_test.go @@ -32,7 +32,7 @@ import ( const ( fakeVersion = "v1.2.3" - podSpecHash = "6cfdc7d895" + podSpecHash = "fc445b4c6" ) func TestTaskPod(t *testing.T) { diff --git a/pkg/controllers/tiflash/tasks/pod_test.go b/pkg/controllers/tiflash/tasks/pod_test.go index b18ef3797e..34080cda69 100644 --- a/pkg/controllers/tiflash/tasks/pod_test.go +++ b/pkg/controllers/tiflash/tasks/pod_test.go @@ -32,7 +32,7 @@ import ( const ( fakeVersion = "v1.2.3" - fakePodHash = "76b9ffc44" + fakePodHash = "5bf69bf8f9" ) func TestTaskPod(t *testing.T) { diff --git a/pkg/controllers/tikv/tasks/pod_test.go b/pkg/controllers/tikv/tasks/pod_test.go index 174c4aed2c..1c1dc6433f 100644 --- a/pkg/controllers/tikv/tasks/pod_test.go +++ b/pkg/controllers/tikv/tasks/pod_test.go @@ -35,7 +35,7 @@ import ( const ( fakeVersion = "v1.2.3" - podSpecHash = "6b85d6945f" + podSpecHash = "5f7b88ffbd" ) func TestTaskPod(t *testing.T) { diff --git a/pkg/utils/k8s/pod.go b/pkg/utils/k8s/pod.go index 4f0b665be5..9a497c31d3 100644 --- a/pkg/utils/k8s/pod.go +++ b/pkg/utils/k8s/pod.go @@ -15,19 +15,34 @@ package k8s import ( - "encoding/json" + "bytes" "fmt" "hash/fnv" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + serializerjson "k8s.io/apimachinery/pkg/runtime/serializer/json" "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/client-go/kubernetes/scheme" "github.com/pingcap/tidb-operator/apis/core/v1alpha1" maputil "github.com/pingcap/tidb-operator/pkg/utils/map" hashutil "github.com/pingcap/tidb-operator/third_party/kubernetes/pkg/util/hash" ) +var podEncoder = scheme.Codecs.EncoderForVersion( + serializerjson.NewSerializerWithOptions( + serializerjson.DefaultMetaFactory, + scheme.Scheme, + scheme.Scheme, + serializerjson.SerializerOptions{ + Yaml: false, + Pretty: false, + Strict: true, + }), + corev1.SchemeGroupVersion, +) + // CalculateHashAndSetLabels calculate the hash of pod spec and set it to the pod labels. func CalculateHashAndSetLabels(pod *corev1.Pod) { spec := pod.Spec.DeepCopy() @@ -38,9 +53,10 @@ func CalculateHashAndSetLabels(pod *corev1.Pod) { } // This prevents the hash from being changed when new fields are added to the `PodSpec` due to K8s version upgrades. - data, _ := json.Marshal(spec) + buf := bytes.Buffer{} + _ = podEncoder.Encode(&corev1.Pod{Spec: *spec}, &buf) hasher := fnv.New32a() - hashutil.DeepHashObject(hasher, data) + hashutil.DeepHashObject(hasher, buf.Bytes()) if pod.Labels == nil { pod.Labels = map[string]string{} diff --git a/pkg/utils/k8s/revision/controller_revision.go b/pkg/utils/k8s/revision/controller_revision.go index e3805f6ede..bfa6da362e 100644 --- a/pkg/utils/k8s/revision/controller_revision.go +++ b/pkg/utils/k8s/revision/controller_revision.go @@ -148,6 +148,7 @@ func getPatch(obj client.Object, gvk schema.GroupVersionKind) ([]byte, error) { }), gvk.GroupVersion(), ) + encoderMap[gvk.GroupVersion()] = encoder } buf := bytes.Buffer{} From c7d4d3a5db36327fc791627d6155a29990cba5b8 Mon Sep 17 00:00:00 2001 From: fgksgf Date: Wed, 15 Jan 2025 19:39:08 +0800 Subject: [PATCH 5/5] address comments --- pkg/utils/k8s/pod.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/utils/k8s/pod.go b/pkg/utils/k8s/pod.go index 9a497c31d3..276c465d1c 100644 --- a/pkg/utils/k8s/pod.go +++ b/pkg/utils/k8s/pod.go @@ -54,7 +54,9 @@ func CalculateHashAndSetLabels(pod *corev1.Pod) { // This prevents the hash from being changed when new fields are added to the `PodSpec` due to K8s version upgrades. buf := bytes.Buffer{} - _ = podEncoder.Encode(&corev1.Pod{Spec: *spec}, &buf) + if err := podEncoder.Encode(&corev1.Pod{Spec: *spec}, &buf); err != nil { + panic(fmt.Errorf("failed to encode pod spec, %w", err)) + } hasher := fnv.New32a() hashutil.DeepHashObject(hasher, buf.Bytes())