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.

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

Fixes BackendTLSPolicy Status Update data race #6185

Conversation

sunjayBhatia
Copy link
Member

DAG cache objects were being written to when StatusUpdater fetched resource to perform update on since status updates held a reference to them.

@sunjayBhatia sunjayBhatia added the release-note/small A small change that needs one line of explanation in the release notes. label Feb 12, 2024
@sunjayBhatia sunjayBhatia requested a review from a team as a code owner February 12, 2024 19:17
@sunjayBhatia sunjayBhatia requested review from tsaarni, skriss, a team, izturn and clayton-gonsalves and removed request for a team February 12, 2024 19:17
@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Feb 12, 2024

See failure: https://github.com/projectcontour/contour/actions/runs/7875556667/job/21487800526?pr=6184

==================
  WARNING: DATA RACE
  Read at 0x00c000c3b4c8 by goroutine 256:
    github.com/projectcontour/contour/internal/dag.(*KubernetesCache).configMapTriggersRebuild()
        /home/runner/work/contour/contour/internal/dag/cache.go:619 +0x169c
    github.com/projectcontour/contour/internal/dag.(*KubernetesCache).remove()
        /home/runner/work/contour/contour/internal/dag/cache.go:326 +0x16b9
    github.com/projectcontour/contour/internal/dag.(*KubernetesCache).Remove()
        /home/runner/work/contour/contour/internal/dag/cache.go:307 +0xf1
    github.com/projectcontour/contour/internal/contour.(*EventHandler).onUpdate()
        /home/runner/work/contour/contour/internal/contour/handler.go:262 +0xf1b
    github.com/projectcontour/contour/internal/contour.(*EventHandler).Start()
        /home/runner/work/contour/contour/internal/contour/handler.go:168 +0x77d
    sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/manager/runnable_group.go:223 +0x1ce
    sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func2()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/manager/runnable_group.go:226 +0x41

  Previous write at 0x00c000c3b4c8 by goroutine 282:
    reflect.typedmemmove()
        /opt/hostedtoolcache/go/1.21.6/x64/src/runtime/mbarrier.go:193 +0x0
    reflect.Value.Set()
        /opt/hostedtoolcache/go/1.21.6/x64/src/reflect/value.go:2265 +0x184
    sigs.k8s.io/controller-runtime/pkg/cache/internal.(*CacheReader).Get()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/cache/internal/cache_reader.go:99 +0x706
    sigs.k8s.io/controller-runtime/pkg/cache.(*informerCache).Get()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/cache/informer_cache.go:88 +0x244
    sigs.k8s.io/controller-runtime/pkg/cache.(*delegatingByGVKCache).Get()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/cache/delegating_by_gvk_cache.go:44 +0x144
    sigs.k8s.io/controller-runtime/pkg/client.(*client).Get()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/client/client.go:356 +0x551
    github.com/projectcontour/contour/internal/k8s.(*StatusUpdateHandler).apply.func3()
        /home/runner/work/contour/contour/internal/k8s/status.go:123 +0x145
    k8s.io/client-go/util/retry.OnError.func1()
        /home/runner/go/pkg/mod/k8s.io/client-go@v0.29.1/util/retry/util.go:51 +0x41
    k8s.io/apimachinery/pkg/util/wait.runConditionWithCrashProtection()
        /home/runner/go/pkg/mod/k8s.io/apimachinery@v0.29.1/pkg/util/wait/wait.go:145 +0x53
    k8s.io/apimachinery/pkg/util/wait.ExponentialBackoff()
        /home/runner/go/pkg/mod/k8s.io/apimachinery@v0.29.1/pkg/util/wait/backoff.go:461 +0x5e
    k8s.io/client-go/util/retry.OnError()
        /home/runner/go/pkg/mod/k8s.io/client-go@v0.29.1/util/retry/util.go:50 +0x144
    github.com/projectcontour/contour/internal/k8s.(*StatusUpdateHandler).apply()
        /home/runner/work/contour/contour/internal/k8s/status.go:113 +0x6b7
    github.com/projectcontour/contour/internal/k8s.(*StatusUpdateHandler).Start()
        /home/runner/work/contour/contour/internal/k8s/status.go:162 +0x2fb
    sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func1()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/manager/runnable_group.go:223 +0x1ce
    sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile.func2()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/manager/runnable_group.go:226 +0x41

  Goroutine 256 (running) created at:
    sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/manager/runnable_group.go:207 +0x250
    sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).Start.func1.2()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/manager/runnable_group.go:136 +0x33

  Goroutine 282 (running) created at:
    sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).reconcile()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/manager/runnable_group.go:207 +0x250
    sigs.k8s.io/controller-runtime/pkg/manager.(*runnableGroup).Start.func1.2()
        /home/runner/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.17.1/pkg/manager/runnable_group.go:136 +0x33

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@fa1d380). Click here to learn what that means.
Report is 1 commits behind head on main.

❗ Current head 927bc41 differs from pull request most recent head 39ccd41. Consider uploading reports for the commit 39ccd41 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6185   +/-   ##
=======================================
  Coverage        ?   79.51%           
=======================================
  Files           ?      142           
  Lines           ?    16243           
  Branches        ?        0           
=======================================
  Hits            ?    12916           
  Misses          ?     3014           
  Partials        ?      313           
Files Coverage Δ
internal/dag/gatewayapi_processor.go 93.32% <ø> (ø)
internal/status/backendtlspolicyconditions.go 91.37% <ø> (ø)
internal/status/cache.go 93.22% <100.00%> (ø)

DAG cache objects were being written to when StatusUpdater
fetched resource to perform update on since status updates
held a reference to them.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
@sunjayBhatia sunjayBhatia force-pushed the fix-backendtlspolicy-status-update-data-race branch from 927bc41 to 39ccd41 Compare February 12, 2024 22:09
@sunjayBhatia sunjayBhatia enabled auto-merge (squash) February 12, 2024 22:15
@sunjayBhatia sunjayBhatia merged commit f131882 into projectcontour:main Feb 12, 2024
24 checks passed
@sunjayBhatia sunjayBhatia deleted the fix-backendtlspolicy-status-update-data-race branch February 12, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants