Fix InitialBuckets() for statefulSetBuilder's electors
#2483
Conversation
Some are for StatefulSet and only owns their own
Codecov Report
@@ Coverage Diff @@
## main #2483 +/- ##
==========================================
+ Coverage 81.32% 81.91% +0.59%
==========================================
Files 163 163
Lines 6499 9619 +3120
==========================================
+ Hits 5285 7879 +2594
- Misses 981 1505 +524
- Partials 233 235 +2
Continue to review full report at Codecov.
|
leaderelection/context.go
Outdated
| bkt := ue.bkt | ||
| if bkt == nil { | ||
| bkt = reconciler.UniversalBucket() | ||
| } | ||
| return []reconciler.Bucket{ | ||
| reconciler.UniversalBucket(), | ||
| bkt, |
There was a problem hiding this comment.
I think you can simply return ue.bkt since the universal bucket is configured here:
Lines 101 to 109 in 8db11d0
One thing to note - Promote is called twice
Here when the initial buckets are seeded. It uses a nil enq function since we're not processing items yet and don't need to re-enqueue stuff.
Lines 471 to 477 in 8db11d0
The other promote happens during the Run() call
Lines 269 to 272 in 8db11d0
I was thinking of dropping this but it changes the behaviour of this package - ie. clients need to know to call Promote themselves.
The notable thing about this promote it pass the enq function - so I'm unsure if this has side effects for statefulset electors
There was a problem hiding this comment.
I see, we could dedup simply by checking if the enq is nil in RunContext()
There was a problem hiding this comment.
I’m not super concerned about multiple promotions if there aren’t enq side effects. Another alternative is my original suggestion to call Promote immediately on the code path where we use the unopposed elector with the universal bucket.
There was a problem hiding this comment.
Ok based on what you both say here I am going to simplify the InitialBuckets() func as recommended and let 2x la.Promote() be since there is no side-effect.
There was a problem hiding this comment.
Another alternative is my original suggestion to call Promote immediately on the code path where we use the unopposed elector with the universal bucket.
Yeah - I didn't do it this way because promoting a bucket during the elector build seemed like a weird side-effect.
There was a problem hiding this comment.
I'm not sure when the original bug went in, but if there's been a release cut since, well, even without it, I'd suggest adding a release note.
There was a problem hiding this comment.
@vaikas no there hasn't been a release cut since then. release cut is next week :)
There was a problem hiding this comment.
@dprotaso I simplified the InitialBuckets() func as suggested.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, tcnghia 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 |
Tekton uses a version of knative/pkg in between a regression on the HA via StatefulSet and its fix. Cherry-pick the patch for a minor release. We shall pick-up a newer version of knative/pkg for the next major release instead. Original Knative PR: knative/pkg#2483 Tekton Issue: tektoncd#3404 Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>
Tekton uses a version of knative/pkg in between a regression on the HA via StatefulSet and its fix. Cherry-pick the patch for a minor release. We shall pick-up a newer version of knative/pkg for the next major release instead. Original Knative PR: knative/pkg#2483 Tekton Issue: #3404 Signed-off-by: Andrea Frittoli <andrea.frittoli@uk.ibm.com>
Changes
statefulSetBuilder's electors get promote to own the universal bucket.Release Note
Docs