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

cacher: when forgeting a watcher, call stopWatcherLocked multiple times #121615

Merged
merged 1 commit into from Oct 31, 2023
Merged

cacher: when forgeting a watcher, call stopWatcherLocked multiple times #121615

merged 1 commit into from Oct 31, 2023

Conversation

p0lyn0mial
Copy link
Contributor

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

It's possible that the watcher is already not in the structure (e.g. in case of simultaneous Stop() and terminateAllWatchers(), but it is safe to call stopLocked() on a watcher multiple times.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@p0lyn0mial
cacher: when forgeting a watcher, call stopWatcherLocked multiple times
bbca4a4 
It's possible that the watcher is already not in the structure (e.g. in case of
simultaneous Stop() and terminateAllWatchers(), but it is safe to call stopLocked()
on a watcher multiple times.
@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 30, 2023
@p0lyn0mial p0lyn0mial marked this pull request as ready for review October 30, 2023 15:23
@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 30, 2023
@p0lyn0mial
Copy link
Contributor Author

/assign @wojtek-t

@k8s-ci-robot k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 30, 2023
@k8s-ci-robot k8s-ci-robot requested review from enj and ncdc October 30, 2023 15:24
@p0lyn0mial p0lyn0mial mentioned this pull request Oct 30, 2023
Closed
@wojtek-t
Copy link
Member

/lgtm
/approve

/kind bug
/priority important-soon

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 31, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: c6ae6f33ff30a2feea2e2330572de2aca985c39f

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2023
@p0lyn0mial
Copy link
Contributor Author

xref: #121438

@k8s-ci-robot k8s-ci-robot merged commit 5bac451 into kubernetes:master Oct 31, 2023
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Oct 31, 2023
@aojea
Copy link
Member

aojea commented Oct 31, 2023

@wojtek-t @p0lyn0mial this causing the CI to fail https://storage.googleapis.com/k8s-triage/index.html?ci=0&pr=1&job=unit , the test add is racy, I suggest we revert this

/cc @liggitt

Failed;
=== RUN   TestForgetWatcher
==================
WARNING: DATA RACE
Write at 0x00c002b71ac0 by goroutine 37417:
  k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher.(*indexedWatchers).terminateAll()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher/cacher.go:175 +0x3c7
  k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher.(*Cacher).terminateAllWatchers()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher/cacher.go:1181 +0x105
  k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher.(*Cacher).startCaching()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher/cacher.go:460 +0x1c5
  k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher.NewCacherFromConfig.func1.1()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher/cacher.go:431 +0x46
  k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:226 +0x41
  k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.BackoffUntil()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:227 +0xbe
  k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.JitterUntil()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:204 +0x10a
  k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait.Until()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:161 +0x149
  k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher.NewCacherFromConfig.func1()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher/cacher.go:428 +0x12f

Previous read at 0x00c002b71ac0 by goroutine 37414:
  k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher.TestForgetWatcher()
      /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/cacher/cacher_whitebox_test.go:1849 +0xaf7
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()

@p0lyn0mial
Copy link
Contributor Author

yes, i have opened #121645 to address the issue. i'm also okay with reverting the pr.

@aojea
Copy link
Member

aojea commented Oct 31, 2023

yes, i have opened #121645 to address the issue. i'm also okay with reverting the pr.

@p0lyn0mial please send a revert just to unblock the CI, and work in parallel on the fix

@rphillips rphillips mentioned this pull request Oct 31, 2023
Merged
@p0lyn0mial p0lyn0mial mentioned this pull request Oct 31, 2023
Merged
@leilajal
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

@enj enj Awaiting requested review from enj

@ncdc ncdc Awaiting requested review from ncdc

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@wojtek-t wojtek-t

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@p0lyn0mial @k8s-ci-robot @wojtek-t @aojea @leilajal