🐛 fix runnable run twice issue#487
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
pkg/manager/internal.go
Outdated
| RetryPeriod: cm.retryPeriod, | ||
| Callbacks: leaderelection.LeaderCallbacks{ | ||
| OnStartedLeading: func(_ context.Context) { | ||
| cm.isLeader = true |
There was a problem hiding this comment.
Setting isLeader needs to be protected by the mutex. I might suggest moving it into startLeaderElectionRunnables and changing its name to something like startedLeader?
There was a problem hiding this comment.
yeap, that makes more sense. 👍
pkg/manager/internal.go
Outdated
| shouldStart = cm.started | ||
| cm.nonLeaderElectionRunnables = append(cm.nonLeaderElectionRunnables, r) | ||
| } else { | ||
| shouldStart = cm.started && (cm.isLeader || cm.resourceLock == nil) |
There was a problem hiding this comment.
If you take the suggestion from the other comment, this could be simplified to shouldStart = cm.startedLeader.
adohe-zz
force-pushed
the
fix_runnable_twice
branch
from
5e3d91f to
e63210a
Compare
|
fyi #444 |
|
The code looks good to me, but it would be nice to see the tests pass -- even though that failure doesn't look like your fault. |
|
/lgtm |
|
@abursavich: changing LGTM is restricted to assignees, and only kubernetes-sigs/controller-runtime repo collaborators may be assigned issues. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@DirectXMan12 kind ping~ |
|
@mengqiy @DirectXMan12 the CI is happy, kind ping~ |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adohe, DirectXMan12 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
5 participants
fix #478 @DirectXMan12 @abursavich ptal