Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

concurrency errors when printing source.TypedSource #3057

Open
nicks opened this issue Jan 3, 2025 · 1 comment · May be fixed by #3068
Open

concurrency errors when printing source.TypedSource #3057

nicks opened this issue Jan 3, 2025 · 1 comment · May be fixed by #3068

Comments

@nicks
Copy link

nicks commented Jan 3, 2025

Repro steps:

  • Write a custom implementation of source.TypedSource
  • Add a Watch on it

Result:
Non-deterministically, controller-runtime blows up with this error:

fatal error: concurrent map iteration and map write

(full stack trace included below)

The Problem

Controller does a fmt.Printf on the Source, which implicitly reads all the internal properties.

controller-runtime/pkg/internal/controller/controller.go

Line 178 in 1ac370e

log := c.LogConstructor(nil).WithValues("source", fmt.Sprintf("%s", watch))

All the built-in Sources implement fmt.Stringer with a concurrency-safe String() string method, so do not have this problem.

Suggestion: the TypedSource interface should also implement fmt.Stringer, so that all implementers have to implement String()

Additional info

Full stack trace:

goroutine 194539 [running]:
reflect.mapiternext(0x18c24c9?)
	/usr/local/go/src/runtime/map.go:1537 +0x13
reflect.(*MapIter).Next(0xc005b6ad38?)
	/usr/local/go/src/reflect/value.go:1989 +0x74
internal/fmtsort.Sort({0x42cf740?, 0xc0202be2c8?, 0xc0267a4380?})
	/usr/local/go/src/internal/fmtsort/sort.go:59 +0x1a7
fmt.(*pp).printValue(0xc012c1b930, {0x42cf740?, 0xc0202be2c8?, 0x5cc9b4207?}, 0x76, 0x3)
	/usr/local/go/src/fmt/print.go:816 +0x988
fmt.(*pp).printValue(0xc012c1b930, {0x4659fe0?, 0xc0202be2a0?, 0xc01f0d7320?}, 0x76, 0x2)
	/usr/local/go/src/fmt/print.go:853 +0x11be
fmt.(*pp).printValue(0xc012c1b930, {0x44e0640?, 0xc0202be2a0?, 0x74cea8f28e70?}, 0x76, 0x1)
	/usr/local/go/src/fmt/print.go:853 +0x11be
fmt.(*pp).printValue(0xc012c1b930, {0x4569ce0?, 0xc0202be2a0?, 0x182a8dd?}, 0x76, 0x0)
	/usr/local/go/src/fmt/print.go:921 +0xae5
fmt.(*pp).badVerb(0xc012c1b930, 0x73)
	/usr/local/go/src/fmt/print.go:394 +0x47f
fmt.(*pp).fmtPointer(0xc012c1b930, {0x4569ce0?, 0xc0202be2a0?, 0x18dcd70?}, 0x73)
	/usr/local/go/src/fmt/print.go:583 +0x414
fmt.(*pp).printValue(0xc012c1b930, {0x4569ce0?, 0xc0202be2a0?, 0x17c6cf0?}, 0x73, 0x2)
	/usr/local/go/src/fmt/print.go:927 +0x1165
fmt.(*pp).printValue(0xc012c1b930, {0x4662ee0?, 0xc02581a9c0?, 0x18dcd70?}, 0x73, 0x1)
	/usr/local/go/src/fmt/print.go:853 +0x11be
fmt.(*pp).printValue(0xc012c1b930, {0x456e1e0?, 0xc02581a9c0?, 0x38?}, 0x73, 0x0)
	/usr/local/go/src/fmt/print.go:921 +0xae5
fmt.(*pp).printArg(0xc012c1b930, {0x456e1e0, 0xc02581a9c0}, 0x73)
	/usr/local/go/src/fmt/print.go:759 +0x4bb
fmt.(*pp).doPrintf(0xc012c1b930, {0x485887b, 0x2}, {0xc005b6beb0, 0x1, 0x1})
	/usr/local/go/src/fmt/print.go:1074 +0x37e
fmt.Sprintf({0x485887b, 0x2}, {0xc00c637eb0, 0x1, 0x1})
	/usr/local/go/src/fmt/print.go:239 +0x53
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2({0x4f37800, 0xc029b08780}, 0xc023ab4790, 0x4f61f00)
	/home/circleci/go/src/github.com/tilt-dev/tilt/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:175 +0x187
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start(0x4f61f00, {0x4f37800, 0xc029b08780})
	/home/circleci/go/src/github.com/tilt-dev/tilt/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:231 +0x186
sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1(0xc02257a840)
	/home/circleci/go/src/github.com/tilt-dev/tilt/vendor/sigs.k8s.io/controller-runtime/pkg/manager/runnable_group.go:226 +0xc2
created by sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile in goroutine 194352
	/home/circleci/go/src/github.com/tilt-dev/tilt/vendor/sigs.k8s.io/controller-runtime/pkg/manager/runnable_group.go:210 +0x19d
nicks added a commit to tilt-dev/tilt that referenced this issue Jan 3, 2025
@nicks
controllers: ensure all sources implement fmt.Stringer
a0f6e56 
prevents race conditions that cause a panic

for more info, see
kubernetes-sigs/controller-runtime#3057

Signed-off-by: Nick Santos <[email protected]>
@nicks nicks mentioned this issue Jan 3, 2025
Merged
nicks added a commit to tilt-dev/tilt that referenced this issue Jan 3, 2025
@nicks
controllers: ensure all sources implement fmt.Stringer (#6481)
03b64de 
prevents race conditions that cause a panic

for more info, see
kubernetes-sigs/controller-runtime#3057

Signed-off-by: Nick Santos <[email protected]>
@troy0820
Copy link
Member

Extending the interface to ensure that this can be printed, would that be something we would want to enforce? Only because the builtins already satisfy that so there would be no breaking changes but only when we release a new version that we can express for those who have custom sources. Thoughts? @alvaroaleman @sbueringer

@troy0820 troy0820 linked a pull request Jan 13, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

⚠Add String() to Source interface to enforce fmt.Stringer interface troy0820/controller-runtime
2 participants
@nicks @troy0820