fix: avoid write-read concurrency on cached cluster by YanniHu1996 · Pull Request #2884 · cloudnative-pg/cloudnative-pg

YanniHu1996 · 2023-09-26T10:32:08Z

Closes: #2863

I reviewed the instance manager log attached in #2860 and identified two significant points

goroutine 562 [running]:
reflect.mapiterinit(0x451652?, 0x1c0?, 0x1d96c40?)
	/opt/hostedtoolcache/go/1.20.6/x64/src/runtime/map.go:1375 +0x19
reflect.(*MapIter).Next(0x1c7fc60?)
	/opt/hostedtoolcache/go/1.20.6/x64/src/reflect/value.go:1919 +0x65
encoding/json.mapEncoder.encode({0xc00062d388?}, 0xc00048c280, {0x1c7fc60?, 0xc0004af678?, 0x800?}, {0xd?, 0x0?})
	/opt/hostedtoolcache/go/1.20.6/x64/src/encoding/json/encode.go:797 +0x33e
encoding/json.structEncoder.encode({{{0xc000b0fb00?, 0x79c025?, 0xc00062d490?}, 0xc0006d9ad0?}}, 0xc00048c280, {0x1e35ae0?, 0xc0004af678?, 0xd?}, {0x0, 0x1})
	/opt/hostedtoolcache/go/1.20.6/x64/src/encoding/json/encode.go:759 +0x1f4
encoding/json.structEncoder.encode({{{0xc0005d8c00?, 0x37?, 0xc00062d598?}, 0xc000683380?}}, 0xc00048c280, {0x1f460a0?, 0xc0004af608?, 0x7?}, {0x0, 0x1})
	/opt/hostedtoolcache/go/1.20.6/x64/src/encoding/json/encode.go:759 +0x1f4
encoding/json.structEncoder.encode({{{0xc000b0e480?, 0xc00062d690?, 0x40bbb6?}, 0xc000683bc0?}}, 0xc00048c280, {0x1e63b60?, 0xc0004af500?, 0x7a025a?}, {0x0, 0x1})
	/opt/hostedtoolcache/go/1.20.6/x64/src/encoding/json/encode.go:759 +0x1f4
encoding/json.ptrEncoder.encode({0x1?}, 0xc00048c280, {0x1f53e00?, 0xc0004af500?, 0x1f53e00?}, {0x8?, 0xd8?})
	/opt/hostedtoolcache/go/1.20.6/x64/src/encoding/json/encode.go:943 +0x21c
encoding/json.(*encodeState).reflectValue(0xc00048c280?, {0x1f53e00?, 0xc0004af500?, 0x40e1c7?}, {0x78?, 0x0?})
	/opt/hostedtoolcache/go/1.20.6/x64/src/encoding/json/encode.go:358 +0x78
encoding/json.(*encodeState).marshal(0xc00062d8f0?, {0x1f53e00?, 0xc0004af500?}, {0x20?, 0x60?})
	/opt/hostedtoolcache/go/1.20.6/x64/src/encoding/json/encode.go:330 +0xfa
encoding/json.Marshal({0x1f53e00, 0xc0004af500})
	/opt/hostedtoolcache/go/1.20.6/x64/src/encoding/json/encode.go:161 +0xe5
github.com/cloudnative-pg/cloudnative-pg/pkg/management/postgres/webserver.(*localWebserverEndpoints).serveCache(0x985a70?, {0x22618d0, 0xc000e4a000}, 0x50fb69?)
	pkg/management/postgres/webserver/local.go:96 +0x179
net/http.HandlerFunc.ServeHTTP(0xc00048c100?, {0x22618d0?, 0xc000e4a000?}, 0x40ddea?)
	/opt/hostedtoolcache/go/1.20.6/x64/src/net/http/server.go:2122 +0x2f
net/http.(*ServeMux).ServeHTTP(0x0?, {0x22618d0, 0xc000e4a000}, 0xc000e7a000)
	/opt/hostedtoolcache/go/1.20.6/x64/src/net/http/server.go:2500 +0x149
net/http.serverHandler.ServeHTTP({0xc0004224e0?}, {0x22618d0, 0xc000e4a000}, 0xc000e7a000)
	/opt/hostedtoolcache/go/1.20.6/x64/src/net/http/server.go:2936 +0x316
net/http.(*conn).serve(0xc00029e090, {0x2262830, 0xc0004b8180})
	/opt/hostedtoolcache/go/1.20.6/x64/src/net/http/server.go:1995 +0x612
created by net/http.(*Server).Serve
	/opt/hostedtoolcache/go/1.20.6/x64/src/net/http/server.go:3089 +0x5ed


goroutine 134 [runnable]:
reflect.Value.Addr({0x1d38640?, 0xc0004af7e8?, 0x195?})
	/opt/hostedtoolcache/go/1.20.6/x64/src/reflect/value.go:271 +0x7a
sigs.k8s.io/json/internal/golang/encoding/json.indirect({0x1d38640?, 0xc0004af7e8?, 0xc0004af698?}, 0x0)
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:473 +0xcf
sigs.k8s.io/json/internal/golang/encoding/json.(*decodeState).object(0xc000b809c0, {0x1d38640?, 0xc0004af7e8?, 0x1dd6c20?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:645 +0x69
sigs.k8s.io/json/internal/golang/encoding/json.(*decodeState).value(0xc000b809c0, {0x1d38640?, 0xc0004af7e8?, 0x6?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:401 +0x45
sigs.k8s.io/json/internal/golang/encoding/json.(*decodeState).object(0xc000b809c0, {0x1dd6c20?, 0xc0004af7e8?, 0x117c?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:867 +0x1310
sigs.k8s.io/json/internal/golang/encoding/json.(*decodeState).value(0xc000b809c0, {0x1dd6c20?, 0xc0004af7e8?, 0x9?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:401 +0x45
sigs.k8s.io/json/internal/golang/encoding/json.(*decodeState).object(0xc000b809c0, {0x1f460a0?, 0xc0004af608?, 0x24d3?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:867 +0x1310
sigs.k8s.io/json/internal/golang/encoding/json.(*decodeState).value(0xc000b809c0, {0x1f460a0?, 0xc0004af608?, 0x4?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:401 +0x45
sigs.k8s.io/json/internal/golang/encoding/json.(*decodeState).object(0xc000b809c0, {0x1f53e00?, 0xc0004af500?, 0x1f53e00?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:867 +0x1310
sigs.k8s.io/json/internal/golang/encoding/json.(*decodeState).value(0xc000b809c0, {0x1f53e00?, 0xc0004af500?, 0x7f14f6210a68?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:401 +0x45
sigs.k8s.io/json/internal/golang/encoding/json.(*decodeState).unmarshal(0xc000b809c0, {0x1f53e00?, 0xc0004af500?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:188 +0x168
sigs.k8s.io/json/internal/golang/encoding/json.Unmarshal({0xc0007f0000, 0x1d2c, 0x2500}, {0x1f53e00, 0xc0004af500}, {0xc000654c68, 0x2, 0xc000654c88?})
	pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/decode.go:113 +0x159
sigs.k8s.io/json.UnmarshalCaseSensitivePreserveInts(...)
	pkg/mod/sigs.k8s.io/[email protected]/json.go:62
k8s.io/apimachinery/pkg/runtime/serializer/json.(*Serializer).unmarshal(0xc0000561f8?, {0x224c7f0?, 0xc0004af500?}, {0xc0007f0000?, 0xc000d4a160?, 0x7?}, {0xc0007f0000?, 0xc00029a770?, 0x2247920?})
	pkg/mod/k8s.io/[email protected]/pkg/runtime/serializer/json/json.go:258 +0x39c
k8s.io/apimachinery/pkg/runtime/serializer/json.(*Serializer).Decode(0xc0000dc230, {0xc0007f0000, 0x1d2c, 0x2500}, 0x0, {0x224c7f0, 0xc0004af500?})
	pkg/mod/k8s.io/[email protected]/pkg/runtime/serializer/json/json.go:206 +0xa6a
k8s.io/apimachinery/pkg/runtime.WithoutVersionDecoder.Decode({{0x2247a00?, 0xc0000dc230?}}, {0xc0007f0000?, 0x199?, 0xc0004af500?}, 0x1e63b60?, {0x224c7f0?, 0xc0004af500?})
	pkg/mod/k8s.io/[email protected]/pkg/runtime/helper.go:252 +0x55
sigs.k8s.io/controller-runtime/pkg/client/apiutil.targetZeroingDecoder.Decode({{0x224b200?, 0xc000b644e0?}}, {0xc0007f0000, 0x1d2c, 0x2500}, 0x0?, {0x224c7f0?, 0xc0004af500?})
	pkg/mod/sigs.k8s.io/[email protected]/pkg/client/apiutil/apimachinery.go:236 +0xb8
k8s.io/client-go/rest.Result.Into({{0xc0007f0000, 0x1d2c, 0x2500}, {0x0, 0x0, 0x0}, {0xc000b2c3b0, 0x10}, {0x0, 0x0}, ...}, ...)
	pkg/mod/k8s.io/[email protected]/rest/request.go:1349 +0xad
sigs.k8s.io/controller-runtime/pkg/client.(*typedClient).PatchSubResource(0xc0005f8510, {0x2262830, 0xc000179da0}, {0x2286488?, 0xc0004af500}, {0x1f593e8, 0x6}, {0x2252e98, 0xc000400b40}, {0x0, ...})
	pkg/mod/sigs.k8s.io/[email protected]/pkg/client/typed_client.go:278 +0x505
sigs.k8s.io/controller-runtime/pkg/client.(*subResourceClient).Patch(0xc0003a0588, {0x2262830, 0xc000179da0}, {0x2286488?, 0xc0004af500}, {0x2252e98, 0xc000400b40}, {0x0, 0x0, 0x0})
	pkg/mod/sigs.k8s.io/[email protected]/pkg/client/client.go:585 +0x315
github.com/cloudnative-pg/cloudnative-pg/internal/management/controller.(*InstanceReconciler).reconcilePrimary(0xc000280e80, {0x2262830, 0xc000179da0}, 0xc0004af500)
	internal/management/controller/instance_controller.go:1066 +0x305
github.com/cloudnative-pg/cloudnative-pg/internal/management/controller.(*InstanceReconciler).Reconcile(0xc000280e80, {0x2262830?, 0xc000179a10?}, {{{0xc000179a10?, 0x0?}, {0xc000655d20?, 0x40e1c7?}}})
	internal/management/controller/instance_controller.go:188 +0x692
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0x2262830?, {0x2262830?, 0xc000179a10?}, {{{0xc0004315b0?, 0x1bb6d60?}, {0xc0006f4c48?, 0x224eac8?}}})
	pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:118 +0xc8
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0xc0002fde00, {0x2262788, 0xc0000dc730}, {0x1d73540?, 0xc000415420?})
	pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:314 +0x377
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0xc0002fde00, {0x2262788, 0xc0000dc730})
	pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:265 +0x1d9
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
	pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:226 +0x85
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2
	pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:222 +0x587

There are concurrent read/write on cluster object

serveCache includes a read operation on cluster object when json marshal
reconcilePrimary includes a write operation on cluster object when calling k8s client to perform a patch

Note that the write operation on the function reconcilePrimary occurs on the case of switchover that is mentioned by the above dicussion

github-actions · 2023-09-26T11:08:30Z

❗ By default, the pull request is configured to backport to all release branches.

To stop backporting this pr, remove the label: backport-requested ◀️ or add the label 'do not backport'
To stop backporting this pr to a certain release branch, remove the specific branch label: release-x.y

YanniHu1996 · 2023-09-26T14:28:28Z

the way to fix this issue is to make a deep copy of the cluster object in store function

doesn't make deep copy in load fubction as the cluster object from load function isn't been modified

armru · 2023-09-27T08:07:43Z

/test limit=local

github-actions · 2023-09-27T08:08:06Z

@armru, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/6323144676

armru · 2023-09-27T08:23:57Z

/test limit=local

github-actions · 2023-09-27T08:24:14Z

@armru, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/6323315479

Signed-off-by: YanniHu1996 <[email protected]>

Signed-off-by: Armando Ruocco <[email protected]>

Closes: #2863 Signed-off-by: YanniHu1996 <[email protected]> Signed-off-by: Armando Ruocco <[email protected]> Co-authored-by: Armando Ruocco <[email protected]> (cherry picked from commit f77af89)

) Closes: cloudnative-pg#2863 Signed-off-by: YanniHu1996 <[email protected]> Signed-off-by: Armando Ruocco <[email protected]> Co-authored-by: Armando Ruocco <[email protected]> Signed-off-by: Lennart Krauch <[email protected]>

YanniHu1996 requested review from armru, fcanovai, gbartolini, leonardoce, mnencia, phisco and sxd as code owners September 26, 2023 10:32

YanniHu1996 force-pushed the dev/cnp-4163 branch from ac9698a to 6e5b8b7 Compare September 26, 2023 10:38

github-actions Bot added backport-requested ◀️ This pull request should be backported to all supported releases release-1.19 labels Sep 26, 2023

armru force-pushed the dev/cnp-4163 branch 2 times, most recently from ed68fb7 to 24dc85a Compare September 27, 2023 08:07

armru force-pushed the dev/cnp-4163 branch 3 times, most recently from cfb24e0 to a9c036a Compare September 27, 2023 08:23

armru changed the title ~~fix: Fixed data race on cached cluster~~ fix: avoid data race on cached cluster Sep 27, 2023

armru changed the title ~~fix: avoid data race on cached cluster~~ fix: avoid write/read concurrency on cached cluster Sep 27, 2023

armru changed the title ~~fix: avoid write/read concurrency on cached cluster~~ fix: avoid write-read concurrency on cached cluster Sep 27, 2023

armru force-pushed the dev/cnp-4163 branch from 25bc005 to 5f8ba05 Compare September 27, 2023 10:17

YanniHu1996 and others added 4 commits September 27, 2023 13:39

fix: Fixed data race on cached cluster

5d6dbdd

Signed-off-by: YanniHu1996 <[email protected]>

chore: fmt

d2e2933

Signed-off-by: Armando Ruocco <[email protected]>

chore: add comment

4971e11

Signed-off-by: Armando Ruocco <[email protected]>

chore: rename load cluster

3abfd08

Signed-off-by: Armando Ruocco <[email protected]>

gbartolini force-pushed the dev/cnp-4163 branch from 5f8ba05 to 3abfd08 Compare September 27, 2023 11:39

gbartolini approved these changes Sep 27, 2023

View reviewed changes

armru approved these changes Sep 27, 2023

View reviewed changes

gbartolini added the ok to merge 👌 This PR can be merged label Sep 27, 2023

gbartolini merged commit f77af89 into cloudnative-pg:main Sep 27, 2023

gbartolini deleted the dev/cnp-4163 branch September 27, 2023 12:18

Navigation Menu

Repository navigation

fix: avoid write-read concurrency on cached cluster - #2884

fix: avoid write-read concurrency on cached cluster#2884
gbartolini merged 4 commits into
cloudnative-pg:mainfrom
EnterpriseDB:dev/cnp-4163

YanniHu1996 commented Sep 26, 2023 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 26, 2023

Uh oh!

YanniHu1996 commented Sep 26, 2023

Uh oh!

armru commented Sep 27, 2023

Uh oh!

github-actions Bot commented Sep 27, 2023

Uh oh!

armru commented Sep 27, 2023

Uh oh!

github-actions Bot commented Sep 27, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

YanniHu1996 commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 26, 2023

Uh oh!

YanniHu1996 commented Sep 26, 2023

Uh oh!

armru commented Sep 27, 2023

Uh oh!

github-actions Bot commented Sep 27, 2023

Uh oh!

armru commented Sep 27, 2023

Uh oh!

github-actions Bot commented Sep 27, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YanniHu1996 commented Sep 26, 2023 •

edited

Loading